Child FFD constraint tests and fixes #103

sseraj · 2021-10-03T23:57:31Z

Purpose

This PR makes several changes related to DVConstraints and child FFDs:

Parameterized the DVConstraints tests to also run them with child FFDs. The parent FFD is a stationary box around the original FFD. We use the original reference files for the child FFD tests, which verifies that the constraint values and derivatives are identical with a fixed parent FFD.
The only test that is not parameterized is the triangulated surface (GeoGrad) test with multiple DVGeos because it is unclear how to handle multiple DVGeos with child FFDs (related to Refactor handling of multiple DVGeo objects with DVCon #57).
Fixed addLinearConstraintsShape and addMonotonicConstraints to work with child FFDs. Linear constraints need a childIdx argument because they operate directly on the FFD.
Added a test for the curvature constraint. This was one of only two constraints that were not tested, the other being the gear post constraint (which I doubt anyone still uses).
Added a test for the LE radius constraint. This was already tested in test_Cylinder.py, but a wing provides a more typical test case. In addition, this test is part of the same parameterized class so it is also tested with child FFDs.
Renamed the DVConstraints tests and reference files to have actual names instead of numbers.

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 code adheres to PEP-8 and is consistently formatted
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

Co-authored-by: Bernardo Pacini <[email protected]>

marcomangano

Nice stuff, this test script was really messed up.
Take a look at my comments, I will re-check the final script tomorrow because it is hard to follow these big commits that move function blocks around

marcomangano · 2021-10-04T22:06:47Z

tests/reg_tests/test_DVConstraints.py

@@ -163,7 +186,11 @@ def c172_test_twist(self, DVGeo, DVCon, handler):
        # change the DVs
        xDV = DVGeo.getValues()
        xDV["twist"] = np.linspace(0, 10, self.nTwist)
-        DVGeo.setDesignVars(xDV)
+        if self.child:
+            # Twist needs to be set on the parent FFD to get accurate derivatives


I can confirm this is the same approach I use, but is this documented anywhere? What is the reason behind this?

I don't think it's documented anywhere mostly because manually setting design variables is not too common. This is not specific to twist actually. My understanding is that setDesignVars should always be called on the parent, which will recursively set the DVs on all children. It just so happens that shape variables can be applied directly on a child FFD without causing issues. I can change the approach for all DVs to be consistent if you think that's clearer.

No I think it makes sense to keep the shape with a different behaviour, as it is a localDV instead of globalDV. Can you just add a one-liner in setDesignVars docstring?

marcomangano · 2021-10-04T22:23:09Z

tests/reg_tests/test_DVConstraints.py

+        handler.root_add_dict("funcs_deformed", funcs, rtol=1e-6, atol=1e-6)
+        handler.root_add_dict("derivs_deformed", funcsSens, rtol=1e-6, atol=1e-6)
+        return funcs, funcsSens
+


Is there a way to directly skip the class initialization down here? no biggie though

Despite being outdated, I still refer to the RegTestGeograd class

You mean skip the class instead of each test if geograd is not installed? Yes, I think that's possible. I will change that.

I don't think we do it in other repos, but if you find a way it could be nice. The initialization time for pyGeo is not that bad, but the solvers might benefit from this?

marcomangano · 2021-10-04T22:36:12Z

pygeo/constraints/DVCon.py

@@ -2007,6 +2017,11 @@ def addLinearConstraintsShape(

        self._checkDVGeo(DVGeoName)

+        if childIdx is not None:
+            DVGeo = self.DVGeometries[DVGeoName].children[childIdx]


It took me a bit of code reading, but I see how this makes sense. We have a single DVcon object which "receives" the parent DVGeo, and we were previously applying the constraint to this object instead of the children (when present) as expected. This means that we now need to add one of these for each child ffd, right? Can you add a short description of what is going on and how to extract the childIdx index from DVGeo?

Yes, without childIdx the constraint is applied on the parent. You only need to add this constraint to each FFD that needs it. childIdx is defined by the order in which you add the child FFD to the parent (the first is 0, second is 1, etc.). I will update the docstrings to include this description.

To be clear though, I took this approach directly from addLeTeConstraints which already worked with child FFDs.

marcomangano · 2021-10-04T22:39:21Z

tests/reg_tests/test_DVConstraints.py

+        """
+        The rectangular box is primarily used to test unscaled constraint function
+        values against known values for thickness, volume, and surface area.
+        """


Nice! Are the comments on the rest of the functions just redundant, and so you opted to remove then? I think we could use a one-liner for all the function here, even if the names are pretty self explanatory

Yes, I felt the comments were redundant. Some were also incorrect (saying that a constraint does not rely on point sets when it does). What did you have in mind for the one-liners? I think they would be almost exactly the same as the test name.

yeah I see, don't worry, this is fine

marcomangano · 2021-10-04T22:50:30Z

tests/reg_tests/test_DVConstraints.py

@@ -732,6 +778,34 @@ def test_monotonic(self, train=False, refDeriv=False):
                atol=1e-7,
            )

+    @unittest.skipIf(missing_geograd, "requires geograd")


So, the other test is on its own only because it cannot be used under parameterized -i.e. switch between parent/child ffd?

Yes, that's right

marcomangano · 2021-10-04T22:51:55Z

tests/reg_tests/test_DVConstraints.py

-            funcs, funcsSens = self.bwb_test_deformed(DVGeo, DVCon, handler)
-
-    @unittest.skipIf(missing_geograd, "requires geograd")
-    def test_triangulatedSurface_intersected(self, train=False, refDeriv=False):


Where did this end up?

That is here. I initially moved all the geograd tests into a separate class but realized I only needed to move the one with 2 DVGeos out.

marcomangano · 2021-10-04T22:54:10Z

tests/reg_tests/test_DVConstraints.py

+    def test_LERadius(self, train=False, refDeriv=False):
+        refFile = os.path.join(self.base_path, "ref/test_DVConstraints_LERadius.ref")
+        with BaseRegTest(refFile, train=train) as handler:
+            DVGeo, DVCon = self.generate_dvgeo_dvcon_c172()


Maybe we can generalize the name of this function as well?

Just the name or the functionality as well? I thought about creating one function that works with all the geometries because this would reduce code duplication. I can take a look at that if think it's worth it.

uh, that would be nice if low-ish effort

Ok, I just checked the functions and I think we should definitely merge generate_dvgeo_dvcon_c172 and generate_dvgeo_dvcon_rect. The only differences are the meshes, the location of the ref axis, the addToDVGeo(?) option in DVCon.setSurface, and the normalization of the testMesh.vectors(??) and they should be pretty easy to handle with an option

sseraj · 2021-10-05T15:02:39Z

Thanks @marcomangano for the thorough review! I will get started on the changes. Feel free to add other suggestions as well

codecov · 2021-10-05T16:40:26Z

Codecov Report

Merging #103 (4b0c2a6) into master (3b92c25) will increase coverage by 3.76%.
The diff coverage is 100.00%.

❗ Current head 4b0c2a6 differs from pull request most recent head fb54855. Consider uploading reports for the commit fb54855 to get more accurate results

@@            Coverage Diff             @@
##           master     #103      +/-   ##
==========================================
+ Coverage   55.58%   59.35%   +3.76%     
==========================================
  Files          40       40              
  Lines        9608     9614       +6     
==========================================
+ Hits         5341     5706     +365     
+ Misses       4267     3908     -359

Impacted Files	Coverage Δ
pygeo/constraints/DVCon.py	`65.14% <100.00%> (+2.84%)`	⬆️
pygeo/parameterization/__init__.py	`60.00% <0.00%> (-40.00%)`	⬇️
pygeo/__init__.py	`73.33% <0.00%> (-26.67%)`	⬇️
pygeo/parameterization/DVGeoVSP.py	`79.91% <0.00%> (-1.12%)`	⬇️
pygeo/topology.py	`54.88% <0.00%> (+0.44%)`	⬆️
pygeo/constraints/radiusConstraint.py	`81.44% <0.00%> (+2.06%)`	⬆️
pygeo/pyGeo.py	`39.74% <0.00%> (+3.37%)`	⬆️
pygeo/constraints/curvatureConstraint.py	`89.63% <0.00%> (+84.19%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7cf8e4e...fb54855. Read the comment docs.

marcomangano · 2021-10-05T17:11:42Z

tests/reg_tests/test_DVConstraints.py

@@ -922,6 +831,7 @@ def test_LERadius(self, train=False, refDeriv=False):
            funcs, funcsSens = self.wing_test_deformed(DVGeo, DVCon, handler)


+@unittest.skipIf(missing_geograd, "requires geograd")


that was easy lol

ArshSaja

Nice work!

sseraj and others added 11 commits September 29, 2021 17:02

parameterizing DVCon tests to use child FFDs

7bc1ebd

Co-authored-by: Bernardo Pacini <[email protected]>

Merge branch 'master' into dvcon-childFFD

0d23ae7

updated parent FFD path

5fb6996

split out geograd tests

74de39a

child FFD fixes for linear constraints

5e06463

renamed tests and ref files, clarified comments

f226e6a

refactoring planarity and tri vol to run with child FFDs

ffac49d

moved single DVGeo geograd tests back to parameterized class

d4671d2

generalized c172 test DVs

949864e

camel case changes

c33030e

added curvature and LE radius tests

ab236b3

sseraj requested a review from a team as a code owner October 3, 2021 23:57

sseraj requested review from hajdik and marcomangano October 3, 2021 23:57

marcomangano reviewed Oct 4, 2021

View reviewed changes

sseraj added 3 commits October 5, 2021 11:09

skip geograd class instead of test

5a65e5a

updated docstrings and kwargs order

058cb27

generalized dvgeo, dvcon generation

fb54855

marcomangano reviewed Oct 5, 2021

View reviewed changes

marcomangano approved these changes Oct 5, 2021

View reviewed changes

ArshSaja approved these changes Oct 5, 2021

View reviewed changes

ArshSaja merged commit 435d7ad into master Oct 5, 2021

sseraj deleted the dvcon-childFFD branch October 5, 2021 18:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Child FFD constraint tests and fixes #103

Child FFD constraint tests and fixes #103

sseraj commented Oct 3, 2021

marcomangano left a comment

marcomangano Oct 4, 2021

sseraj Oct 5, 2021

marcomangano Oct 5, 2021

marcomangano Oct 4, 2021

marcomangano Oct 4, 2021

sseraj Oct 5, 2021

marcomangano Oct 5, 2021

marcomangano Oct 4, 2021

sseraj Oct 5, 2021

marcomangano Oct 4, 2021

sseraj Oct 5, 2021 •

edited

Loading

marcomangano Oct 5, 2021

marcomangano Oct 4, 2021 •

edited

Loading

sseraj Oct 5, 2021

marcomangano Oct 4, 2021

sseraj Oct 5, 2021

marcomangano Oct 4, 2021

sseraj Oct 5, 2021 •

edited

Loading

marcomangano Oct 5, 2021

marcomangano Oct 5, 2021

sseraj commented Oct 5, 2021

codecov bot commented Oct 5, 2021 •

edited

Loading

marcomangano Oct 5, 2021

ArshSaja left a comment

		@@ -922,6 +831,7 @@ def test_LERadius(self, train=False, refDeriv=False):
		funcs, funcsSens = self.wing_test_deformed(DVGeo, DVCon, handler)


		@unittest.skipIf(missing_geograd, "requires geograd")

Child FFD constraint tests and fixes #103

Child FFD constraint tests and fixes #103

Conversation

sseraj commented Oct 3, 2021

Purpose

Type of change

Testing

Checklist

marcomangano 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

Choose a reason for hiding this comment

sseraj Oct 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcomangano Oct 4, 2021 • edited Loading

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

sseraj Oct 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sseraj commented Oct 5, 2021

codecov bot commented Oct 5, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

ArshSaja left a comment

Choose a reason for hiding this comment

sseraj Oct 5, 2021 •

edited

Loading

marcomangano Oct 4, 2021 •

edited

Loading

sseraj Oct 5, 2021 •

edited

Loading

codecov bot commented Oct 5, 2021 •

edited

Loading