Error if derivative mode other than "rev" is requested for `OM_DVGEOCOMP`

[eytanadler]

Purpose

The MPhys component for pygeo supports only reverse mode derivatives and otherwise gives zero derivatives. However, no warning or error is thrown. This PR adds a RuntimeError if the derivatives are requested with a mode other than "rev".

Expected time until merged

A few days

Type of change

• [ ] Bugfix (non-breaking change which fixes an issue)
• [ ] New feature (non-breaking change which adds functionality)
• [ ] Breaking change (non-backwards-compatible fix or feature)
• [ ] Code style update (formatting, renaming)
• [ ] Refactoring (no functional changes, no API changes)
• [ ] Documentation update
• [x] Maintenance update
• [ ] Other (please describe)

Testing

Checklist

• [ ] I have run flake8 and black to make sure the Python code adheres to PEP-8 and is consistently formatted
• [ ] I have formatted the Fortran code with fprettify or C/C++ code with clang-format as applicable
• [ ] I have run unit and regression tests which pass locally with my changes
• [ ] I have added new tests that prove my fix is effective or that my feature works
• [ ] I have added necessary documentation

[codecov[bot]]

Codecov Report

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

Project coverage is 67.03%. Comparing base (191046d) to head (ec99fe4). Report is 5 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #250      +/-   ##
==========================================
+ Coverage   65.41%   67.03%   +1.61%     
==========================================
  Files          47       47              
  Lines       12315    12331      +16     
==========================================
+ Hits         8056     8266     +210     
+ Misses       4259     4065     -194     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: 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.

[eirikurj]

@eytanadler, this was discussed during the maintenance meeting last week (missed the last one), but just want to clarify, do you plan to add the forward mode in this PR? We can also just add that in a separate PR, but in that case please make an issue documenting the forward mode additions etc.

[eytanadler]

Yes, I’ll add it here. I just haven’t got around to it yet.

[eytanadler]

I did a first pass of forward mode derivatives for the MPhys DVGeo component. I'm not super familiar with the parallelization hacks, so @anilyil please check those and let me know if you have any concerns. Tests for this component and its derivatives are included on the mphys-tests branch, so I think it'd be best to either merge that branch into this one or this branch into that one. That way, we can ensure these derivatives work as expected before merging to main. I ran the tests locally and this seems to produce the same results as reverse mode.

A couple other things I found and changed while making this:

• The check for missing variables in totalSensitivityProd incorrectly finds missing variables if the variable is not in the final DVGeo. I fixed this by initializing the set with the variables we want to find rather than adding them as we go.
• The tests on the mphys-tests branch identify an incorrect derivative in the projected area constraint that is not tested elsewhere in the code. I looked into it for awhile and couldn't nail down the error. However, I did find one case where the inequality check in the derivative calculation is if XYZ > 0 [do something] else [do something else], whereas in the primal it is if XYZ < 0 [do something else] else [do something]. I switched the derivative version around so it matches the primal. @hajdik it'd be good for us to get to the bottom of this derivative problem. Nonetheless, the fwd and rev modes have the same problem, so I don't think it's something I introduced in this code.

In the meantime, I'll add fwd mode derivative checking to the tests on the mphys-tests branch.

[timryanb]

What's the status of this PR @eytanadler? I made my own implementation of fwd mode (PR #254), but looking at this PR in further detail you appear to have already added your own implementation. Is this working? If so, I can close my PR.

[eytanadler]

What's the status of this PR @eytanadler? I made my own implementation of fwd mode (PR #254), but looking at this PR in further detail you appear to have already added your own implementation. Is this working? If so, I can close my PR.

@timryanb this PR is ready to go, but the tests for it are on the mphys-tests branch. @hajdik and I were planning on wrapping that up and merging with this, but then got sidetracked with other PhD work. One of the tricky things with that work was we found an incorrect derivative and spent some time digging to figure out why, but to no avail, which is one of the reasons it got held up.

As an aside, are there any details from your forward mode implementation that I missed here or that would improve this PR's implementation?

[timryanb]

What's the status of this PR @eytanadler? I made my own implementation of fwd mode (PR #254), but looking at this PR in further detail you appear to have already added your own implementation. Is this working? If so, I can close my PR.

@timryanb this PR is ready to go, but the tests for it are on the mphys-tests branch. @hajdik and I were planning on wrapping that up and merging with this, but then got sidetracked with other PhD work. One of the tricky things with that work was we found an incorrect derivative and spent some time digging to figure out why, but to no avail, which is one of the reasons it got held up.

As an aside, are there any details from your forward mode implementation that I missed here or that would improve this PR's implementation?

No, I think our approaches should be functionally equivalent. I tried running your branch on a test case of mine and it ran as expected, so I think I'll just close my PR.

[eytanadler]

This is finally ready for review

[marcomangano]

Is this change worth version update @eytanadler @hajdik ? Feel free to merge otherwise

[eytanadler]

Yeah probably a good idea