Skip to content

Conversation

@FerreolS
Copy link
Contributor

Still have to update the README

Copy link

@gdalle gdalle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Congrats on the refactor!
Have you looked into the sparse Hessian capabilities of DI? These might be of interest to you

Project.toml Outdated

[weakdeps]
Zygote = "e88e6eb3-aa80-5325-afca-941959d7151f"
DifferentiationInterface = "a0c0ee7d-e4b9-4e03-894e-1c5f64a51d63"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that DifferentiationInterface is so lightweight is doesn't really make sense to make it a weakdep.

julia> @time_imports using DifferentiationInterface
      1.9 ms  ADTypes
               ┌ 0.0 ms DifferentiationInterface.__init__() 
     43.4 ms  DifferentiationInterface 94.19% compilation time

Comment on lines 12 to 18
if isdefined(Base, :get_extension)
using DifferentiationInterface
using OptimPackNextGen
else
using ..DifferentiationInterface
using ..OptimPackNextGen
end
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is no longer needed with Julia 1.10

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point is to keep backward compatibility with julia version <1.10.
In any case, I'll have to update those lines

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that 1.10 is the new LTS, these language versions are now unmaintained, and most packages have probably dropped them by now. It is debatable whether you also need to ensure support (note that users can still install older versions of your package if you drop it).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we have quit old code/package that hadn't been updated for years we need to keep such backward compatibily (albeit without AD).
From my test everything but AD works down to 1.6

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then I would suggest you don't deactivate the CI on those older Julia versions, but instead make the AD support/tests conditional on the Julia version?
Note that support for Julia 1.6 was dropped in DifferentiationInterface v0.6.8, so if you downgrade your compat bound to DI v0.6.7 it should work there too.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then I would suggest you don't deactivate the CI on those older Julia versions, but instead make the AD support/tests conditional on the Julia version?
Note that support for Julia 1.6 was dropped in DifferentiationInterface v0.6.8, so if you downgrade your compat bound to DI v0.6.7 it should work there too.

That's a good idea but is it really possible given that DifferentiationInterface must appear in the test = [...] list of Project.toml? At least this is my undrestanding.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't be an issue, as long as your compat bound in Project.toml allows at least one version of DI that supports Julia 1.6, it will also work in the test environment

Project.toml Outdated
ArrayTools = "0.2.3, 0.3"
ForwardDiff = "0.10"
Mooncake = "0.4"
Requires = "1"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requires is no longer needed with Julia 1.10

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants