-
Notifications
You must be signed in to change notification settings - Fork 12
feat: Use .jac fields #510
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
base: main
Are you sure you want to change the base?
Conversation
48c27b2 to
99b4260
Compare
| Aggregates the Jacobians stored in the ``.jac`` fields of ``params`` and accumulates the result | ||
| into their ``.grad`` fields. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we set on "Accumulate" vs "store" in .grad?
I think that accumulate is the way to make it more torchy, and store is a new direction related to our (my?) dislike of sums.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the main reason to accumulate is to be more torch-like. Also, if a user really wants to replace the .grad, they can do optimizer.zero_grad() and then call jac_to_grad(...).
The other way around (we decide that jac_to_grad replaces the .grad field and a user instead wants to accumulate) is not doable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty convinced, but let's keep this discussion for few days in case we think of other things.
|
We have something a bit tricky: both To reuse some code in two different packages (
|
|
Lastly, it's very annoying to have this Pylance error all over the place: I'm not sure how we could reliably stop showing this error, or even use a custom tensor that would properly fix this. |
I think this is just because all this is bad design. We made it more torch compatible, and this is the price. Pylance is right, there is not field |
Yeah but there are still ways to make it much cleaner. I'm working on something. |
I think we should keep in mind that we would like the users to have a good typing experience. It would be nasty to make them use |
712667c to
7dda66f
Compare
Well, they should rarely need to use these |
|
right, that's very good, and if they do, they should get some amount of warning. You can delete the discussion if you want (annoying that we cannot have a thread). |
- Remove test_aggregate.py - Update test_accumulate.py and test_interactions.py to test on AccumulateGrad instead of Accumulate - Fix tests in test_backward.py and test_mtl_backward.py to match the new interface: check the jac field instead of the .grad field. - Use _asserts.py for helper functions common to backward.py and mtl_backward.py
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
This is just a draft, showing more or less how this change will impact the interface. What do you think so far @PierreQuinton ?
TODO: