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

Add the GTPSA.jl backend #329

Merged
merged 54 commits into from
Jan 27, 2025
Merged

Add the GTPSA.jl backend #329

merged 54 commits into from
Jan 27, 2025

Conversation

mattsignorelli
Copy link
Contributor

@gdalle
Copy link
Member

gdalle commented Jun 25, 2024

@mattsignorelli thank you for the contribution!
I fixed a few things and defined AutoGTPSA in the main package for now. Types should not be defined in package extensions because it makes them hard to import.
The main part missing is the pushforward operator. If you don't have a dedicated mechanism for that, you can always deduce it from derivative, gradient or jacobian depending on the function type (array/scalar -> array/scalar). It will be slow but it will work.

@gdalle gdalle linked an issue Jun 25, 2024 that may be closed by this pull request
@gdalle gdalle added the backend Related to one or more autodiff backends label Jun 25, 2024
@codecov-commenter
Copy link

codecov-commenter commented Jun 25, 2024

Codecov Report

Attention: Patch coverage is 0% with 416 lines in your changes missing coverage. Please review.

Project coverage is 54.75%. Comparing base (d51fc0a) to head (77e49d6).

Files with missing lines Patch % Lines
...ace/ext/DifferentiationInterfaceGTPSAExt/onearg.jl 0.00% 287 Missing ⚠️
...ace/ext/DifferentiationInterfaceGTPSAExt/twoarg.jl 0.00% 119 Missing ⚠️
...face/ext/DifferentiationInterfaceGTPSAExt/utils.jl 0.00% 9 Missing ⚠️
...erfaceGTPSAExt/DifferentiationInterfaceGTPSAExt.jl 0.00% 1 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (d51fc0a) and HEAD (77e49d6). Click for more details.

HEAD has 38 uploads less than BASE
Flag BASE (d51fc0a) HEAD (77e49d6)
DI 41 11
DIT 10 2
Additional details and impacted files
@@             Coverage Diff             @@
##             main     #329       +/-   ##
===========================================
- Coverage   98.51%   54.75%   -43.76%     
===========================================
  Files         108      103        -5     
  Lines        4297     4597      +300     
===========================================
- Hits         4233     2517     -1716     
- Misses         64     2080     +2016     
Flag Coverage Δ
DI 52.44% <0.00%> (-46.14%) ⬇️
DIT 60.01% <ø> (-38.37%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@gdalle gdalle marked this pull request as draft June 25, 2024 09:05
@gdalle gdalle marked this pull request as ready for review July 8, 2024 04:41
@gdalle gdalle marked this pull request as draft July 30, 2024 14:05
@mattsignorelli
Copy link
Contributor Author

Sorry for the delay on this, I've had my hands tied with all different projects. Two weeks ago I pushed major updates/fixes and finishing touches to GTPSA.jl, which is now it's in first stable release. I'll get back to this shortly

@gdalle
Copy link
Member

gdalle commented Aug 7, 2024

The structure of the tests has changed a little since the last time you checked, can you merge main into this PR?

@mattsignorelli
Copy link
Contributor Author

@gdalle This should be ready soon.. For HVP, I'm having difficulty understanding the tangents. For example one of the tests gives a 6x6 Hessian matrix but the tangent is a scalar number. Shouldn't the tangent be a vector?

@mattsignorelli
Copy link
Contributor Author

OK other than hvp, we are pretty much done, all other tests pass. In order to achieve full code coverage, I'm adding the kwarg unsafe_inbounds to GTPSA's getter functions, so that I can run tests with AutoGTPSA{Descriptor}, and just let the Descriptor have many variables to second order (before was just running tests with AutoGTPSA{Nothing}). Once this is released I will bump the version.

I'm having a bit of trouble with hvp. It's basically the same code that worked some number of months ago, but now it's showing disagreements. Any help or clarification on my comment above would be much appreciated :) On that note, I looked into whether or not there is a faster way to calculate the hvp without materializing the full hessian, and unfortunately because TPSs cannot be nested, there isn't really any other option than materializing the full hessian. There are some other ways I found, but none of them are faster..

@mattsignorelli
Copy link
Contributor Author

mattsignorelli commented Jan 8, 2025

Actually, I realized I made a mistake in which tests are being run when I made my previous comment. I found the problem

@mattsignorelli mattsignorelli marked this pull request as ready for review January 8, 2025 14:48
@mattsignorelli mattsignorelli requested a review from gdalle as a code owner January 8, 2025 14:48
@mattsignorelli
Copy link
Contributor Author

This is ready for merge now

@gdalle
Copy link
Member

gdalle commented Jan 11, 2025

thanks, I'll take a look!

@mattsignorelli
Copy link
Contributor Author

Hi any updates on this?

Copy link
Member

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

Thank you and sorry for the delay! Here are my remarks.
I don't understand 100% of the code but I'm confident enough in my test suite and in your work, so if the last few comments are addressed, I will happily merge

@gdalle
Copy link
Member

gdalle commented Jan 23, 2025

Note that you can commit the changes in a batch instead of one by one, by going to the "Files changed" tab

@mattsignorelli
Copy link
Contributor Author

OK thank you for your thorough review!

The only remaining points to check are:

  1. Documentation of "variables" is sufficient
  2. That gradient can take Any?
  3. Clarification on the Context question

@gdalle
Copy link
Member

gdalle commented Jan 23, 2025

Documentation of "variables" is sufficient

Yeah, I think the link to the GTPSA docs is enough. Btw, I was wondering what the point of include_params=true is?

That gradient can take Any?

For reverse-mode backends, yes, gradient applies to an arbitrary struct. For forward-mode backends, we can only guarantee its behavior on AbstractArray (that's what ForwardDiff offers). So typing x::AbstractArray is fine.

Clarification on the Context question

Yeah I think you should replace Context with Constant everywhere

@mattsignorelli
Copy link
Contributor Author

Yeah, I think the link to the GTPSA docs is enough. Btw, I was wondering what the point of include_params=true is?

GTPSA also allows one to specify differentials for "parameters", and those can be truncated at a different order than for the "variables". The include_params kwarg just ensures that the result includes the partial derivatives for those differentials.

The distinguishing of "variables" and "parameters" is because if you have say a 3rd order map representing the transport of some particle beam through a machine ("variables" are deviations from the reference particle orbit), and then you can optimize certain higher order partial derivatives of this map using "parameters" to just first order.

For reverse-mode backends, yes, gradient applies to an arbitrary struct. For forward-mode backends, we can only guarantee its behavior on AbstractArray (that's what ForwardDiff offers). So typing x::AbstractArray is fine.

Ah, I understand now. I'll remove these lines for 100% coverage then

Yeah I think you should replace Context with Constant everywhere

OK!

@mattsignorelli
Copy link
Contributor Author

This should be ready to go

Copy link
Member

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

thank you and congrats on seeing this through!

@gdalle gdalle merged commit 8dcd924 into JuliaDiff:main Jan 27, 2025
50 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend Related to one or more autodiff backends
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adding GTPSA.jl to the interface
3 participants