SciML / ADTypes.jl

Repository for automatic differentiation backend types
https://sciml.github.io/ADTypes.jl/
MIT License
37 stars 11 forks source link

Use `Base.@kwdef` #13

Closed prbzrg closed 1 year ago

prbzrg commented 1 year ago

https://docs.julialang.org/en/v1/base/base/#Base.@kwdef

prbzrg commented 1 year ago

I tested it locally, tests passed.

(ADTypes) pkg> test
     Testing ADTypes
      Status `C:\Users\Hossein Pourbozorg\AppData\Local\Temp\jl_qjqKnd\Project.toml`
  [47edcb42] ADTypes v0.1.6 `C:\Users\Hossein Pourbozorg\Code Projects\github-repos\ADTypes.jl`
  [8dfed614] Test `@stdlib/Test`
      Status `C:\Users\Hossein Pourbozorg\AppData\Local\Temp\jl_qjqKnd\Manifest.toml`
  [47edcb42] ADTypes v0.1.6 `C:\Users\Hossein Pourbozorg\Code Projects\github-repos\ADTypes.jl`
  [2a0f44e3] Base64 `@stdlib/Base64`
  [b77e0a4c] InteractiveUtils `@stdlib/InteractiveUtils`
  [56ddb016] Logging `@stdlib/Logging`
  [d6f4376e] Markdown `@stdlib/Markdown`
  [9a3f8284] Random `@stdlib/Random`
  [ea8e919c] SHA v0.7.0 `@stdlib/SHA`
  [9e88b42a] Serialization `@stdlib/Serialization`
  [8dfed614] Test `@stdlib/Test`
     Testing Running tests...
Test Summary: | Pass  Total  Time
ADTypes.jl    |   43     43  0.2s
     Testing ADTypes tests passed

julia> versioninfo()
Julia Version 1.9.2
Commit e4ee485e90 (2023-07-05 09:39 UTC)
Platform Info:
  OS: Windows (x86_64-w64-mingw32)
  CPU: 12 × Intel(R) Core(TM) i7-10750H CPU @ 2.60GHz
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-14.0.6 (ORCJIT, skylake)
  Threads: 13 on 12 virtual cores

Why do they fail here?

Vaibhavdixit02 commented 1 year ago

That's because the ci script has 1.0 instead of 1 and hence a julia 1.0.x is getting used. I have updated that in #15 but maybe you can do it here, one of us will have to rebase and update 😛

prbzrg commented 1 year ago

Oh, now I see it 😁. I updated the CI file.

Vaibhavdixit02 commented 1 year ago

I am curious though why do you think we should change to this instead of the constructors we have right now?

prbzrg commented 1 year ago

IMO, using a helper macro makes the code easier to read; and since Julia 1.9 @kwdef is an exported macro. I guess because it's popular! Performance-wise, it literally translates to the same thing. So there is no difference :

julia> @macroexpand Base.@kwdef struct AutoReverseDiff <: AbstractADType
           compile::Bool = false
       end
quote
    #= util.jl:589 =#
    begin
        $(Expr(:meta, :doc))
        struct AutoReverseDiff <: AbstractADType
            #= REPL[2]:2 =#
            compile::Bool
        end
    end
    #= util.jl:590 =#
    AutoReverseDiff(; compile = false) = begin
            #= util.jl:567 =#
            AutoReverseDiff(compile)
        end
end
Vaibhavdixit02 commented 1 year ago

Cool, thanks for the response! I have used it in the past in another package but that was a while back, and haven't used it since hence just wanted your thought behind this

devmotion commented 9 months ago

This PR broke the package on Julia < 1.6 < 1.1. IMO it's fine to use Base.@kwdef but the problem is that the Julia compat entry was not updated.

prbzrg commented 9 months ago

@kwdef was added to v1.1 based on docs and v1.1 release notes CI doesn't test Julia < 1.6, so if we want them, we should add them to be sure we don't break on them.

devmotion commented 9 months ago

As I said, I'm fine with supporting only Julia >= 1.6. The problem with this PR is that it disabled CI for 1.0 (since tests failed) but did not adjust the compat entry accordingly. This broke at least Julia 1.0.

prbzrg commented 9 months ago

Oh, now I see it. Thanks for noticing it.