Embedding tolerance update #116

marcomangano · 2022-02-15T16:24:17Z

Purpose

The arbitrary tolerance currently set to determine if a point is embedded when interiorOnly is True (children FFDs most commonly use this option) was creating issues in some of my cases, leaving some points stationary as the rest of the mesh/geometry object is deformed:

This PR addressed this issue doing two things:

Updates the tolerance embTol to 1e-10 instead of eps * 50 (with eps being the tolerance for the Newton search in the projection subroutine in pySpline) and decouples it from eps;
enables the user to pass embTol as an argument for easier tuning of the mesh deformation properties.

Note that, as attachPoints is called from addPointSet, which is in turn usually called from other solvers methods (see how it's done in ADflow), other sibling PRs should be opened to propagate the new option to the user level.

Co-authored with @sseraj

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

codecov · 2022-02-15T16:34:46Z

Codecov Report

Merging #116 (fdc2273) into master (743c55c) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

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

@@            Coverage Diff             @@
##           master     #116      +/-   ##
==========================================
- Coverage   63.86%   63.86%   -0.01%     
==========================================
  Files          41       41              
  Lines       10932    10931       -1     
==========================================
- Hits         6982     6981       -1     
  Misses       3950     3950

Impacted Files	Coverage Δ
pygeo/parameterization/DVGeo.py	`66.13% <100.00%> (ø)`
pygeo/pyBlock.py	`47.02% <100.00%> (-0.11%)`	⬇️

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 743c55c...fe012ee. Read the comment docs.

sseraj

This looks fine to me, but I'll leave the approvals to others because I was involved in making the changes

eirikurj

Minor comments, let me know if this is unclear.

eirikurj · 2022-02-16T18:13:21Z

pygeo/pyBlock.py

        nIter = kwargs.get("nIter", 100)
+        eps = kwargs.get("eps", 1e-12)


Dont see the need to expand these here, as they are not used/needed in this function (see the comment below).
Also, this is creating "artificial" defaults for the following function, instead of using the func default.

eirikurj · 2022-02-16T18:16:17Z

pygeo/pyBlock.py

+                volID, u, v, w, D = self.projectPoints(
+                    coordinates, eps=eps, checkErrors=True, nIter=nIter, embTol=embTol
+                )
                self.embeddedVolumes[ptSetName] = EmbeddedVolume(volID, u, v, w)
            else:
-                volID, u, v, w, D = self.projectPoints(coordinates, checkErrors=False, eps=eps, nIter=nIter)
+                volID, u, v, w, D = self.projectPoints(
+                    coordinates, eps=eps, checkErrors=False, nIter=nIter, embTol=embTol
+                )


Since this function already takes in **kwargs (and keywords are not changed and passed to next func) why not just pass it on as **kwargs here instead of expanding and passing explicitly nIter and eps?

eirikurj · 2022-02-16T22:51:48Z

pygeo/parameterization/DVGeo.py

@@ -600,6 +613,8 @@ def addPointSet(self, points, ptName, origConfig=True, **kwargs):

        """

+        embTol = kwargs.get("embTol", 1e-10)  # see pyBlock.attachPoints() documentation


Dont think this is needed here as its not used (see comment below for pyBlock on kwargs)

eirikurj · 2022-02-16T22:52:44Z

pygeo/parameterization/DVGeo.py

+            self.FFD.attachPoints(self.points[ptName], ptName, interiorOnly=True, embTol=embTol, **kwargs)
        else:
-            self.FFD.attachPoints(self.points[ptName], ptName, interiorOnly=False)
+            self.FFD.attachPoints(self.points[ptName], ptName, interiorOnly=False, embTol=embTol, **kwargs)


Same here. See other comments on kwargs

marcomangano · 2022-02-17T16:00:49Z

@eirikurj : We were waiting for a feedback on that! @sseraj and I spent some time discussing about it, and the final criteria we picked was to have the docstring for the argument (which then must be passed explicitly into the function) where the argument is actually used.

As embTol is used in attachPoints(), we expand it and pass it from the higher level - same way we do for nIter and eps in attachPoints() before passing them to projectPoints(). No strong opinion here, we were just looking for a consistent approach that puts the docstrings where the attributes are actually used.

Hope this is clear, if you still prefer to move all the expansions down to one level we can do it.

eirikurj · 2022-02-18T14:23:23Z

@marcomangano I agree that the appropriate docstrings should be where the function parameters are defined/given, and thats all fine.
But, I just dont see the need to extract the values just to pass it on explicitly, unless its being used in that function or has some purpose, which it is not in this case. I feel this is not what the user expects, and is providing different defaults if the value is not given through the kwags. In that case, I would prefer an explicit function signature instead of relying on kwargs. There are however some implications by both approaches.
Instead of writing something long here, I think we could have a quick chat offline to discuss this in case I am missing something. I can also make quick edits and just push to a separate branch to show you what I had in mind.

eirikurj · 2022-02-21T15:08:39Z

@marcomangano I pushed what we discussed offline, here are the two branches that have the changes/suggestions that we talked about. Would be good to have some discussion.

Suggestion 1 - This propagates the kwargs downstream. Benefits are that you dont have to define the downstream parameters like eps in other functions, they only appear in the function that they are defined. The downside is that potentially garbage kwargs are propagated all downstream, (like faceFreeze in the test).
Suggestion 2 - This has only addPointSet to take in kwargs since that is the only one that needs it, but the rest has explicit signatures. The benefits here that everything is explicit, and all defaults defined in the function. The downside is that the defaults are defined in several places.

marcomangano · 2022-02-21T18:56:33Z

@marcomangano I pushed what we discussed offline, here are the two branches that have the changes/suggestions that we talked about. Would be good to have some discussion.

* [Suggestion 1](https://github.com/mdolab/pygeo/compare/ffd-bugfix...eirikurj:ffd-bugfix-patch1) - This propagates the `kwargs` downstream. Benefits are that you dont have to define the downstream parameters like `eps` in other functions, they only appear in the function that they are defined. The downside is that potentially garbage `kwargs` are propagated all downstream, (like `faceFreeze` in the test).

* [Suggestion 2](https://github.com/mdolab/pygeo/compare/ffd-bugfix...eirikurj:ffd-bugfix-patch2) - This has only `addPointSet` to take in `kwargs` since that is the only one that needs it, but the rest has explicit signatures. The benefits here that everything is explicit, and all defaults defined in the function. The downside is that the defaults are defined in several places.

Thanks for putting this together!!! From my point of view option 1 is better - and we can add a warning/check on **kwargs to be sure the user is aware of the garbage kwargs. What do you think about it?

* Properly passing kwargs * More fixes * Remove removed variable in tests Co-authored-by: Eirikur Jonsson <eirikurj@umich.edu>

marcomangano · 2022-02-22T19:21:45Z

@eirikurj check the second-to-last commit, if we remove **kwargs from projectpoints() we should catch any unwanted kwarg in that function

eirikurj · 2022-02-22T21:18:29Z

@marcomangano I dont think you can remove the **kwargs from projectPoints since addPointSet needs to be able to take in whatever to support the DVGeoMulti. The tests are failing due to that.

sseraj · 2022-02-22T22:24:35Z

@eirikurj After some discussion, we decided to go for an approach closer to your Suggestion 2. Following a suggestion from @nwu63, I removed all keyword arguments from projectPoints to avoid duplicate definitions. All kwargs are now defined in attachPoints. Please check that this makes sense.

@marcomangano This should also catch unused kwargs. Popping compNames in addPointSet explicitly excludes it.

eirikurj · 2022-02-23T10:43:19Z

pygeo/parameterization/DVGeo.py

@@ -600,6 +613,9 @@ def addPointSet(self, points, ptName, origConfig=True, **kwargs):

        """

+        # compNames is only needed for DVGeometryMulti, so remove it if passed
+        kwargs.pop("compNames", None)


Just a minor concern. DVGeoMulti has more arguments than just compNames, like applyIC and comm. While no constraints that are calling addPointSet are including these parameters at the moment, they may. Should we pop them here? If not, the following call to attachPoints will fail. To avoid that, you are basically back to my Suggestion 2. Not sure if this is a concern. @sseraj you may have more insights into if that is possibly needed for the constraints when using DVGeoMulti.

I am fine with popping all the unnecessary kwargs here

I don't anticipate adding the other kwargs like comm and applyIC to the constraints. If we do end up adding them in the future, we can also pop them. I would like to keep this list to a minimum though just so we catch any potential errors.

marcomangano · 2022-02-25T15:51:11Z

@joanibal could you take a look at this?

joanibal · 2022-02-28T14:35:48Z

Sure, sorry I didn't know who was responsible for this review

joanibal

One small request

joanibal · 2022-02-28T15:33:34Z

pygeo/pyBlock.py


        # Project Points, if some were actually passed in:
        if coordinates is not None:
            if not interiorOnly:
-                volID, u, v, w, D = self.projectPoints(coordinates, checkErrors=True, eps=eps, nIter=nIter)
+                volID, u, v, w, D = self.projectPoints(coordinates, True, embTol, eps, nIter)


Can we get the CheckErrors=True back the in the function call, otherwise one has to look at projectPoints to see what the True is doing.

We had quite a long discussion about removing kwargs there, I would rather add a local var checkErrors : bool in this function and pass that instead of the bare True of False - would it work?

I like the idea of adding a checkErrors local variable. My proposal would be to do checkErrors = not interiorOnly and move the projectPoints calls outside the if not interiorOnly block.

joanibal · 2022-02-28T15:33:47Z

pygeo/pyBlock.py

                self.embeddedVolumes[ptSetName] = EmbeddedVolume(volID, u, v, w)
            else:
-                volID, u, v, w, D = self.projectPoints(coordinates, checkErrors=False, eps=eps, nIter=nIter)


same as above

joanibal · 2022-02-28T15:34:29Z

pygeo/pyBlock.py

@@ -823,7 +818,7 @@ def attachPoints(self, coordinates, ptSetName, interiorOnly=False, faceFreeze=No
    #             Geometric Functions
    # ----------------------------------------------------------------------

-    def projectPoints(self, x0, eps=1e-12, checkErrors=True, nIter=100):
+    def projectPoints(self, x0, checkErrors, embTol, eps, nIter):


See earlier comments about keeping checkErrors a key word arg

marcomangano · 2022-02-28T16:16:46Z

@eirikurj @sseraj @joanibal check out the updated function and let me know if this is clearer and cleaner. I now just wonder... should we add some kind of message at least, if we call attachPoints(None) ? Because right now nothing happens, and I am a bit confused about the scenario where we call attachPoints.. without points!

sseraj · 2022-02-28T16:24:50Z

There are some scenarios where we have empty point sets (like in parallel when some procs have points and others don't). I think this is fine as is because attachPoints is only used internally.

sseraj

@marcomangano I think you can merge this. The last change was fairly minor.

marcomangano added 3 commits February 14, 2022 16:41

cleanup and comments

3e31964

added new argument for default embedding tolerance

40be723

Merge remote-tracking branch 'origin/master' into ffd-bugfix

dfb9ce5

marcomangano requested a review from sseraj February 15, 2022 16:24

marcomangano added 2 commits February 15, 2022 14:08

fixed docstrings and removed unused arg

0f48770

updated internal API for consistency

4afd290

marcomangano marked this pull request as ready for review February 15, 2022 19:33

marcomangano requested a review from a team as a code owner February 15, 2022 19:33

marcomangano requested review from joanibal and hajdik February 15, 2022 19:33

sseraj mentioned this pull request Feb 15, 2022

kwargs for addPointSet mdolab/adflow#192

Merged

12 tasks

sseraj reviewed Feb 16, 2022

View reviewed changes

marcomangano requested review from ewu63 and anilyil February 16, 2022 16:29

eirikurj requested changes Feb 17, 2022

View reviewed changes

marcomangano and others added 3 commits February 22, 2022 13:03

Ffd bugfix patch1 (#117)

26deec6

* Properly passing kwargs * More fixes * Remove removed variable in tests Co-authored-by: Eirikur Jonsson <eirikurj@umich.edu>

fixed kwargs

d057b18

fix

d52d22b

sseraj added 3 commits February 22, 2022 16:59

removed kwargs entirely from projectPoints

b11495c

update docstrings

abd4250

catch unused kwargs

3f5d694

eirikurj reviewed Feb 23, 2022

View reviewed changes

eirikurj previously approved these changes Feb 24, 2022

View reviewed changes

joanibal requested changes Feb 28, 2022

View reviewed changes

refactoring logic for attachPoints

b4919c1

marcomangano dismissed eirikurj’s stale review via b4919c1 February 28, 2022 16:14

joanibal approved these changes Feb 28, 2022

View reviewed changes

sseraj requested a review from eirikurj February 28, 2022 19:43

Merge branch 'master' into ffd-bugfix

fe012ee

sseraj approved these changes Mar 2, 2022

View reviewed changes

marcomangano merged commit 80cf705 into master Mar 2, 2022

marcomangano deleted the ffd-bugfix branch March 2, 2022 22:14

marcomangano mentioned this pull request Sep 16, 2022

Point volume projection fixes mdolab/pyspline#47

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Embedding tolerance update #116

Embedding tolerance update #116

marcomangano commented Feb 15, 2022 •

edited

codecov bot commented Feb 15, 2022 •

edited

sseraj left a comment

eirikurj left a comment

eirikurj Feb 16, 2022

eirikurj Feb 16, 2022

eirikurj Feb 16, 2022

eirikurj Feb 16, 2022

marcomangano commented Feb 17, 2022

eirikurj commented Feb 18, 2022

eirikurj commented Feb 21, 2022

marcomangano commented Feb 21, 2022

marcomangano commented Feb 22, 2022

eirikurj commented Feb 22, 2022

sseraj commented Feb 22, 2022

eirikurj Feb 23, 2022

marcomangano Feb 23, 2022

sseraj Feb 23, 2022

marcomangano commented Feb 25, 2022

joanibal commented Feb 28, 2022

joanibal left a comment

joanibal Feb 28, 2022

marcomangano Feb 28, 2022

sseraj Feb 28, 2022

marcomangano Feb 28, 2022

joanibal Feb 28, 2022

joanibal Feb 28, 2022

marcomangano commented Feb 28, 2022

sseraj commented Feb 28, 2022

sseraj left a comment

		nIter = kwargs.get("nIter", 100)
		eps = kwargs.get("eps", 1e-12)

		@@ -600,6 +613,8 @@ def addPointSet(self, points, ptName, origConfig=True, **kwargs):

		"""

		embTol = kwargs.get("embTol", 1e-10) # see pyBlock.attachPoints() documentation

Embedding tolerance update #116

Embedding tolerance update #116

Conversation

marcomangano commented Feb 15, 2022 • edited

Purpose

Type of change

Testing

Checklist

codecov bot commented Feb 15, 2022 • edited

Codecov Report

sseraj left a comment

Choose a reason for hiding this comment

eirikurj 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

marcomangano commented Feb 17, 2022

eirikurj commented Feb 18, 2022

eirikurj commented Feb 21, 2022

marcomangano commented Feb 21, 2022

marcomangano commented Feb 22, 2022

eirikurj commented Feb 22, 2022

sseraj commented Feb 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcomangano commented Feb 25, 2022

joanibal commented Feb 28, 2022

joanibal 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

marcomangano commented Feb 28, 2022

sseraj commented Feb 28, 2022

sseraj left a comment

Choose a reason for hiding this comment

marcomangano commented Feb 15, 2022 •

edited

codecov bot commented Feb 15, 2022 •

edited