Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ForwardDiff support #17

Merged
merged 6 commits into from
Oct 12, 2023
Merged

ForwardDiff support #17

merged 6 commits into from
Oct 12, 2023

Conversation

gdalle
Copy link
Contributor

@gdalle gdalle commented Oct 11, 2023

Fix #13 by ensuring compatibility with Dual numbers in a package extension.

For now, method overloads only include:

  • promotion rules
  • exp constructors

This supersedes the WIP package https://github.com/gdalle/ForwardDiffOverLogarithmicNumbers.jl

Ping @cjdoris for review, and @longemen3000 who suggested making this an extension in https://github.com/gdalle/ForwardDiffOverLogarithmicNumbers.jl/pull/1

@gdalle gdalle closed this Oct 11, 2023
@gdalle gdalle reopened this Oct 11, 2023
@longemen3000
Copy link

I updated ForwardDiffOverMeasurements to cover more rules. I had problems with ternary rules (muladd, that is used by evalpoly), so I mimicked some internal ForwardDiff macros to allow for more rule coverage

@gdalle
Copy link
Contributor Author

gdalle commented Oct 12, 2023

Good to know! Is the function you use in the tests the one who hit the most undefined rules? Just to know which one to steal

@longemen3000
Copy link

Not really, but there are just a handful of ternary functions. I really need more tests haha

On this specific package, I would recommend taking special care for functions on LogExpFunctions.jl, maybe it need additional rules

@codecov
Copy link

codecov bot commented Oct 12, 2023

Codecov Report

Merging #17 (17c2478) into main (84151f5) will decrease coverage by 1.17%.
The diff coverage is 77.77%.

@@            Coverage Diff             @@
##             main      #17      +/-   ##
==========================================
- Coverage   98.68%   97.51%   -1.17%     
==========================================
  Files           1        2       +1     
  Lines         304      322      +18     
==========================================
+ Hits          300      314      +14     
- Misses          4        8       +4     
Files Coverage Δ
ext/LogarithmicNumbersForwardDiffExt.jl 77.77% <77.77%> (ø)

@cjdoris
Copy link
Owner

cjdoris commented Oct 12, 2023

Cool!

We need a compat entry for the weakdep, or it won't register.

Could also use PackageExtensionCompat to have this work on earlier Julia versions?

@cjdoris
Copy link
Owner

cjdoris commented Oct 12, 2023

The tests fail on Julia 1.6 because it doesn't load the extension. So either don't run the test on older Julia or use PackageExtensionCompat?

@gdalle
Copy link
Contributor Author

gdalle commented Oct 12, 2023

Could also use PackageExtensionCompat to have this work on earlier Julia versions?

I was wondering about that: do you want a Requires-based extension loading or nothing at all on earlier Julia versions?

@cjdoris
Copy link
Owner

cjdoris commented Oct 12, 2023

Could also use PackageExtensionCompat to have this work on earlier Julia versions?

I was wondering about that: do you want a Requires-based extension loading or nothing at all on earlier Julia versions?

Even though I wrote that package, I'm inclined to suggest only using it as an upgrade route for packages already using Requires. We could just document that the ForwardDiff stuff only works on Julia 1.9+.

@gdalle
Copy link
Contributor Author

gdalle commented Oct 12, 2023

Skipped tests on <1.9 and added a mention in the README. The question of how much stuff to overload remains, but maybe this is good to merge for now and if people have issues they will know where to put more methods?

@cjdoris
Copy link
Owner

cjdoris commented Oct 12, 2023

Yep agreed. Looks good.

The Aqua tests are failing on 1.6 because it wants the Project.toml keys to be sorted 🙄, can you fix that please?

Still needs a compat entry for ForwardDiff too please.

@gdalle
Copy link
Contributor Author

gdalle commented Oct 12, 2023

We should be good to go

@gdalle
Copy link
Contributor Author

gdalle commented Oct 12, 2023

The Aqua inconsistency is an open issue:

JuliaTesting/Aqua.jl#105 (comment)

@gdalle
Copy link
Contributor Author

gdalle commented Oct 12, 2023

crap, I had tested it locally though
looking into it

@cjdoris cjdoris merged commit 4e6203a into cjdoris:main Oct 12, 2023
2 of 4 checks passed
@cjdoris
Copy link
Owner

cjdoris commented Oct 12, 2023

Cheers. I'll make a release tomorrow when I'm not on my phone.

@cjdoris
Copy link
Owner

cjdoris commented Oct 13, 2023

@cjdoris
Copy link
Owner

cjdoris commented Oct 13, 2023

You may also be interested in this package I've been experimenting with: https://github.com/cjdoris/HugeNumbers.jl

@gdalle
Copy link
Contributor Author

gdalle commented Oct 13, 2023

I was already one of your three stars ;) I like basically everything you code in Julia

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.

Compatibility with ForwardDiff
3 participants