mdolab / pygeo

pyGeo provides geometric design variables and constraints suitable for gradient-based optimization.
https://mdolab-pygeo.readthedocs-hosted.com/en/latest/?badge=latest
Apache License 2.0
122 stars 54 forks source link

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

Open eytanadler opened 1 month ago

eytanadler commented 1 month ago

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

Testing

Checklist

codecov[bot] commented 1 month ago

Codecov Report

Attention: Patch coverage is 9.09091% with 30 lines in your changes missing coverage. Please review.

Project coverage is 65.39%. Comparing base (3404b51) to head (65d20c6).

Files Patch % Lines
pygeo/mphys/mphys_dvgeo.py 0.00% 30 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #250 +/- ## ========================================== - Coverage 65.47% 65.39% -0.08% ========================================== Files 47 47 Lines 12265 12280 +15 ========================================== Hits 8030 8030 - Misses 4235 4250 +15 ```

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

eirikurj commented 1 month ago

@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 commented 1 month ago

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

eytanadler commented 1 month ago

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:

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