`OM_DVGEOCOMP` tests and forward mode derivatives #250

eytanadler · 2024-07-07T18:17:51Z

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 forward mode derivatives to the MPhys component so it supports mode="fwd".

The branch for testing the MPhys wrapper for DVGeo/DVCon was merged into this in order to test the forward mode derivatives. The tests so far include commonly used functions in DVGeo, DVCon, and DVGeoESP.

This also addresses the ESP side of #249.

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
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 · 2024-07-07T18:40:38Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.03%. Comparing base (191046d) to head (4712171).

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

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

hajdik

Thanks for catching this, LGTM

eirikurj · 2024-07-18T11:43:23Z

@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 · 2024-07-18T11:51:40Z

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

eytanadler · 2024-07-19T15:59:10Z

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.

…deriv

eytanadler · 2024-10-28T13:22:05Z

This is finally ready for review

lamkina

Thanks for the hard work @hajdik and @eytanadler . Just a few small changes, but overall this is great.

lamkina · 2024-11-07T03:31:54Z

pygeo/mphys/mphys_dvgeo.py

@@ -215,6 +219,9 @@ def nom_addPointSet(self, points, ptName, add_output=True, DVGeoName=None, **kwa
            # add an output to the om component
            self.add_output(ptName, distributed=True, val=points.flatten())

+        if isinstance(DVGeo, DVGeometryESP):
+            return dMax_global


I prefer to not use conditional returns, can we tweak this to set dMaxGlobal equal to None if the DVGeo type is not DVGeometryESP? Then just return the variable without the if-statement.

lamkina · 2024-11-07T03:34:15Z

pygeo/mphys/mphys_dvgeo.py

+                                    # check if this dv is present
+                                    if k in d_inputs:
+                                        # do the allreduce
+                                        # TODO remove the allreduce when this is fixed in openmdao


I think we can remove this TODO. MPhys and OM dev teams decided that individual codes, not OM, are responsible for correctly performing the allreduce in the matrix-free computation.

lamkina · 2024-11-07T03:35:47Z

pygeo/mphys/mphys_dvgeo.py

+                                        xdotg[k] = self.comm.allreduce(xdot[k], op=MPI.SUM)
+
+                                        # accumulate in the dict
+                                        # TODO


If this is working well I think we can clean up this comment as well. If there's actually plans to fix and move back to totalSensitivityTransProd then we should create and issue and reference this code in the issue. Otherwise this comment will probably get lost to the ether.

lamkina · 2024-11-07T03:44:08Z

tests/reg_tests/commonUtils.py

+                i_weight = (n_chord - ii - 1) / (n_chord - i_center - 1)
+
+            # get the direction vectors with unit length
+            dir_up = np.array([0.0, 1.0, 0.0])


Are these Cartesian direction vectors? Should they be an input? I'm not sure the +y and -y will always be considered up/down for all topologies.

Disregard if I'm misunderstanding this.

lamkina · 2024-11-07T03:48:39Z

tests/reg_tests/test_MPhysGeo.py

+
+# input files for all DVGeo types
+input_path = os.path.dirname(os.path.abspath(__file__))
+outerFFD = os.path.join(input_path, "../../input_files/outerBoxFFD.xyz")


Should change "../../input_files/etc" to os.path.join(input_path, "..", "..", "input_files", etc.)

The whole point of using the join function is to eliminate the / character.

Going even further, I like pathlib much better, but that's extra credit.

lamkina · 2024-11-07T03:51:57Z

tests/reg_tests/test_MPhysGeo.py

+        self.prob.run_model()
+
+        totals = self.prob.check_totals(step=1e-7, out_stream=None)
+        assert_check_totals(totals)


Can we add hard-coded tolerances for this check as well? Better to have them here than use the defaults from OM incase they change in the future.

lamkina · 2024-11-07T03:52:54Z

tests/reg_tests/test_MPhysGeo.py

+        paramKwargs.update(kwargs)
+
+        meshFile = os.path.join(input_path, "../../input_files/2x1x8_rectangle.stl")
+        ffdFile = os.path.join(input_path, "../../input_files/2x1x8_rectangle.xyz")


See path comment above.

lamkina · 2024-11-07T03:53:08Z

tests/reg_tests/test_MPhysGeo.py

+        self.prob.run_model()
+
+        totals = self.prob.check_totals(step=1e-7, out_stream=None)
+        assert_check_totals(totals)


We should add tolerances here.

lamkina · 2024-11-07T03:54:57Z

pygeo/mphys/mphys_dvgeo.py

-        DVGeo.addPointSet(points.reshape(len(points) // 3, 3), ptName, **kwargs)
+        if isinstance(DVGeo, DVGeometryESP):
+            # DVGeoESP can return a value to check the pointset distribution
+            dMax_global = DVGeo.addPointSet(points.reshape(len(points) // 3, 3), ptName, **kwargs)


Keep this as camel case: dMaxGlobal

hajdik and others added 6 commits July 2, 2024 10:46

parameterized mphys test format

754b588

Use assert_check_totals

789ab36

DVCon tests for most common functions on a box geometry

0c5e392

Removed unused assert_check_partials import

8946952

Error if any derivative mode other than rev is selected

9d1a470

Fixed if check

01dca80

eytanadler requested a review from a team as a code owner July 7, 2024 18:17

eytanadler requested review from hajdik and ArshSaja July 7, 2024 18:17

Sorry black

1026304

hajdik previously approved these changes Jul 10, 2024

View reviewed changes

start esp test

b5c7035

eytanadler added 2 commits July 19, 2024 11:45

Fix missing var check in totalSensitivityProd

8cf2e26

Made projected area derivative inequality check consistent with primal

1dd8558

eytanadler dismissed hajdik’s stale review via 1dd8558 July 19, 2024 15:46

Add fwd mode derivatives to OM_DVGEOCOMP

65d20c6

eytanadler requested review from lamkina and anilyil and removed request for ArshSaja July 19, 2024 15:50

eytanadler requested a review from hajdik July 19, 2024 15:59

eytanadler and others added 6 commits July 19, 2024 12:06

Check both forward and reverse mode derivatives

35e9b70

Removed explicit mode setting when not doing derivative test

6f2e02e

Merge branch 'mphys-tests' of github.com:mdolab/pygeo into mphys-tests

9bb6d32

Reverse mode DVGeo derivatives should actually use reverse mode

c6af0bd

Merge branch 'mphys-tests' of github.com:mdolab/pygeo into mphys-tests

4c69161

Modify DVCon test FD step size

58debb9

hajdik and others added 22 commits October 17, 2024 12:16

add parameter from DVGeoESP

53102a7

fix ESP test

5a6c42a

Merge branch 'mphys-tests' into mphys_deriv

d0bf709

Adjust tol

f3b7cee

Fix formatting

86c2926

Please flake

5dc7071

Fix bad python

7603a7b

Ignore unused import

7f0cf59

Flake 8 fixes

b61fafd

Remove previous uneeded ignore

fc39ee9

isort

1b4b27a

first try

7fadb24

first try for sure

6aa0781

sure

0668152

?

e2d742f

This one will work

a321803

Remove directional kwarg that was unused and broke old OM versions

fb15b8b

take out parallel case - test not set up for it

4871a3d

Merge branch 'mphys_deriv' of github.com:eytanadler/pygeo into mphys_…

c790303

…deriv

You're not gonna believe this one cool trick

e735915

Merge branch 'mphys_deriv' of github.com:eytanadler/pygeo into mphys_…

6eb7b53

…deriv

Isort is the devil

d441801

hajdik changed the title ~~Implement forward mode derivatives in OM_DVGEOCOMP~~ OM_DVGEOCOMP tests and forward mode derivatives Oct 28, 2024

hajdik requested review from marcomangano and removed request for anilyil and hajdik November 4, 2024 17:07

lamkina requested changes Nov 7, 2024

View reviewed changes

Address Andrew's comments

4712171

eytanadler requested a review from lamkina November 12, 2024 14:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`OM_DVGEOCOMP` tests and forward mode derivatives #250

`OM_DVGEOCOMP` tests and forward mode derivatives #250

eytanadler commented Jul 7, 2024 •

edited by hajdik

Loading

codecov bot commented Jul 7, 2024 •

edited

Loading

hajdik left a comment

eirikurj commented Jul 18, 2024

eytanadler commented Jul 18, 2024

eytanadler commented Jul 19, 2024

eytanadler commented Oct 28, 2024

lamkina left a comment

lamkina Nov 7, 2024

lamkina Nov 7, 2024

lamkina Nov 7, 2024

lamkina Nov 7, 2024

lamkina Nov 7, 2024

lamkina Nov 7, 2024

lamkina Nov 7, 2024

lamkina Nov 7, 2024

lamkina Nov 7, 2024

OM_DVGEOCOMP tests and forward mode derivatives #250

Are you sure you want to change the base?

OM_DVGEOCOMP tests and forward mode derivatives #250

Conversation

eytanadler commented Jul 7, 2024 • edited by hajdik Loading

Purpose

Expected time until merged

Type of change

Testing

Checklist

codecov bot commented Jul 7, 2024 • edited Loading

Codecov Report

hajdik left a comment

Choose a reason for hiding this comment

eirikurj commented Jul 18, 2024

eytanadler commented Jul 18, 2024

eytanadler commented Jul 19, 2024

eytanadler commented Oct 28, 2024

lamkina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

`OM_DVGEOCOMP` tests and forward mode derivatives #250

`OM_DVGEOCOMP` tests and forward mode derivatives #250

eytanadler commented Jul 7, 2024 •

edited by hajdik

Loading

codecov bot commented Jul 7, 2024 •

edited

Loading