Skip to content

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

❌ Patch coverage is 84.61538% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.60%. Comparing base (01329c0) to head (5fd7793).
⚠️ Report is 11 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.60%   -1.22%     
==========================================
  Files          10       10              
  Lines        1572     1659      +87     
==========================================
+ Hits         1412     1470      +58     
- Misses        160      189      +29     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@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.

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 ? :)

@stevengj
Copy link
Member

Some failing CI tests, but the logs have expired and they need to be re-run.

julia = "1.6"
LinearAlgebra = "1.6"
ChainRulesCore = "1"
julia = "1.6"
Copy link
Member

Choose a reason for hiding this comment

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

package extensions require Julia 1.9, I think?

uuid = "5c889d49-8c60-4500-9d10-5d3a22e2f4b9"
authors = ["smataigne <[email protected]> and contributors"]
version = "1.0"
version = "1.0.0"
Copy link
Member

Choose a reason for hiding this comment

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

should be a version bump to 1.1 for a new feature.

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