non-breaking, easier to diff version of gradient-jacbian PR

[ExpandingMan]

This is an alternate version of #1733 as requested there. This version should be non-breaking in terms of API (though I'll still have to work through old julia versions if it doesn't work there).

Please note! Basically the whole idea of #1733 was to make the internals more consistent, which this PR must deliberately spoil in order to not break! Therefore there are some pieces of this which are quite wonky but which make more sense when the gradient and jacobian return behavior is made consistent.

I have left as comments most of what would be needed to change to the behavior of #1733, the rest of the changes are the trivial re-arrangement of the files, so these two PR's together I believe fulfill the requests made there.

This does NOT include the new unit tests in #1733 of which some, but not all, would fail.

[codecov-commenter]

:warning: Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 96.35%. Comparing base (4a5dbba) to head (72aefd1).

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #1760       +/-   ##
===========================================
+ Coverage   71.24%   96.35%   +25.10%     
===========================================
  Files          32        9       -23     
  Lines       13657      411    -13246     
===========================================
- Hits         9730      396     -9334     
+ Misses       3927       15     -3912     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[ExpandingMan]

I've added a bunch of tests that I think cover all the cases we really care about for gradient and jacobian. Please let me know if you feel more are still needed.

I have also renamed derivative to unstructured_derivative. It seems like there is currently no standard way to document functions as "private" or "internal" but documented. I think it would be better to consider unstructured_derivative to not be part of the external API, at least until or unless these functions are made more general to accommodate Reverse.

[ExpandingMan]

I'm working on adding all the various edge cases, but I've hit a bit of a snag. The $\partial\textrm{matrix}/\partial\textrm{vector}$ case is failing with some very strange stack overflow errors inside autodiff. So far I'm having a very hard time understanding how I could have introduced these, but (unlike all other cases) I cannot reproduce them on latest tagged.

It could be some unrelated change that happened more recently than latest tagged. Will take me a bit to sort this out.

[ExpandingMan]

Ah, I have confirmed that I did NOT introduce these errors, they are happening on latest commits to main. I will explicitly mark them out for you in the PR. You may want to test them out and fix them before trying to merge this.

[wsmoses]

Hm maybe in this case it’s wise to split up this PR even further to just add the tests (which confirm current main), then we add this PR stop that

On Mon, Sep 2, 2024 at 11:22 AM ExpandingMan @.***> wrote:

Ah, I have confirmed that I did NOT introduce these errors, they are happening on latest commits to main. I will explicitly mark them out for you in the PR. You may want to test them out and fix them before trying to merge this.

— Reply to this email directly, view it on GitHub https://github.com/EnzymeAD/Enzyme.jl/pull/1760#issuecomment-2325061441, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJTUXGDRHU2WL6DZI6CR5TZUSGFVAVCNFSM6AAAAABNLJLR5KVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMRVGA3DCNBUGE . You are receiving this because you commented.Message ID: <EnzymeAD/Enzyme. @.***>

[ExpandingMan]

I suppose that's not hard to do, but there is a slight caveat: there is a trivial mistake in which a ReverseMode constructor is being called with too few type parameters in main which breaks a whole bunch of cases which this PR also fixes. This is not breaking because it was clearly a bug in the first place, but it would cause a whole bunch of these tests to fail. I think the easiest thing to do would be to fix those (very trivial) errors in the tests PR so as not having to mark a whole bunch of reverse mode stuff as broken.

[wsmoses]

Yeah that might as well also go in the test PR too

On Mon, Sep 2, 2024 at 11:30 AM ExpandingMan @.***> wrote:

I suppose that's not hard to do, but there is a slight caveat: there is a trivial mistake in which a ReverseMode constructor is being called with too few type parameters in main which breaks a whole bunch of cases which this PR also fixes. This is not breaking because it was clearly a bug in the first place, but it would cause a whole bunch of these tests to fail. I think the easiest thing to do would be to fix those (very trivial) errors in the tests PR so as not having to mark a whole bunch of reverse mode stuff as broken.

— Reply to this email directly, view it on GitHub https://github.com/EnzymeAD/Enzyme.jl/pull/1760#issuecomment-2325073359, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJTUXHTKW5EU4O6MEXHQADZUSHA3AVCNFSM6AAAAABNLJLR5KVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMRVGA3TGMZVHE . You are receiving this because you commented.Message ID: <EnzymeAD/Enzyme. @.***>

[wsmoses]

I believe this is since superceeded by API fixes on main, reopen and let me know if otherwise.