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

Automatic differentiation of pfaffian #143

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

Gertian
Copy link

@Gertian Gertian commented Sep 25, 2024

As mentioned in #142 we implemented backwards differentiation for the pfaffian in this PR.

To do this we created a package extension stored which is automatically loaded when chainsrulecore is. This way there is minimal overhead when the user does not need this functionality.

Special thank to @https://github.com/lkdvos for figuring out a proper test using the build in ChainsRuleTestUtils package !

@Gertian Gertian marked this pull request as draft September 25, 2024 15:02
@Gertian
Copy link
Author

Gertian commented Sep 25, 2024

For now I marked this PR as a draft since I'm still figuring out if all required functionality has been added.

In particular I'm still struggling with things such as : ``gradient(x->pfaffian(x), skewhermitian(rand(4,4))[ [1,2],[1,2] ])'' this might however not be a problem.

Copy link

codecov bot commented Sep 25, 2024

Codecov Report

Attention: Patch coverage is 84.61538% with 2 lines in your changes missing coverage. Please review.

Project coverage is 88.73%. Comparing base (01329c0) to head (5a44cda).
Report is 8 commits behind head on main.

Files with missing lines Patch % Lines
ext/SkewLinearAlgebraChainRulesCoreExt.jl 84.61% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #143      +/-   ##
==========================================
- Coverage   89.82%   88.73%   -1.09%     
==========================================
  Files          10       11       +1     
  Lines        1572     1660      +88     
==========================================
+ Hits         1412     1473      +61     
- Misses        160      187      +27     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Gertian
Copy link
Author

Gertian commented Sep 25, 2024

The 1.6 versions are failing because we are using package extensions which were not yet included before v1.9.
This does not affect the performance apart from the AD part though !

Nightly fails for unknown reasons.

Codecov is slightly lowered but I don't see a way to get it back to where it was.

I'll put this PR into ready for review now.

@Gertian Gertian marked this pull request as ready for review September 25, 2024 16:03
@Gertian Gertian marked this pull request as draft September 25, 2024 16:38
@Gertian
Copy link
Author

Gertian commented Sep 25, 2024

I just noticed that something is still wrong with the constructor. I'll try to figure this out tomorrow.

test/chainrulestests.jl Outdated Show resolved Hide resolved
test/chainrulestests.jl Outdated Show resolved Hide resolved
Gertian and others added 3 commits September 26, 2024 08:27
@Gertian
Copy link
Author

Gertian commented Sep 26, 2024

@stevengj , thanks for your input.

I'll use the extension for a few days to see if any more unexpected bugs pop up. If not I think this is ready for merging if you also agree.

@Gertian
Copy link
Author

Gertian commented Sep 27, 2024

I've been using this for a few days now and don't seem to notice any bugs , I'll tag this ready for review.

@Gertian Gertian marked this pull request as ready for review September 27, 2024 19:55
@Gertian
Copy link
Author

Gertian commented Oct 8, 2024

@stevengj so what do we do with this ? :)

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