Make ref axis optional when adding a child FFD #150
Conversation
Codecov Report
@@ Coverage Diff @@
## main #150 +/- ##
===========================================
- Coverage 63.72% 53.27% -10.46%
===========================================
Files 47 47
Lines 11676 11689 +13
===========================================
- Hits 7441 6227 -1214
- Misses 4235 5462 +1227
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
pygeo/parameterization/DVGeo.py
Outdated
| @@ -1685,6 +1686,15 @@ def update(self, ptSetName, childDelta=True, config=None): | |||
| # We've postponed things as long as we can...do the finalization. | |||
| self._finalize() | |||
|
|
|||
| for iChild in range(len(self.children)): | |||
| if len(self.children[iChild].axis) > 0: | |||
| self.children[iChild]._finalize() | |||
There was a problem hiding this comment.
As you mentioned, the _finalize here is needed to get the refAxis.coef (coefficient of refAxis nodes) that are then embedded in the parent control volume below.
I am wondering then:
- if it still makes sense to have another
child._finalize()here - what caused the mismatch in the previous tests - was it due to the fact that the control points themselves were not embedded or something? I am honestly a bit confused by how pyGeo currently works, maybe there are some other intermediate steps we are missing if the axis is not added rightaway
There was a problem hiding this comment.
- The finalize method exists right away if it has already been called. This it is safe to repeatedly call finalize during each call of the update method.
There was a problem hiding this comment.
- I don't think I know which tests you are referring to.
There was a problem hiding this comment.
I was referring to the test failures after the first commit, but the build log is lost now.
Agreed on the multiple finalizing calls.
moved ref axis addition after the FFD coef are set to their original values to ensure ref axis is embedded in the undeformed volume.
| # Make sure coefficients are complex | ||
| self._complexifyCoef() | ||
|
|
||
| # Set all coef Values back to initial values | ||
| if not self.isChild: | ||
| self.FFD.coef = self.origFFDCoef.copy() | ||
| self._setInitialValues() | ||
|
|
||
| for iChild in range(len(self.children)): |
There was a problem hiding this comment.
Good catch here, did not realize that the children were trying to initialize themselves again. Out of curiosity, did you have issues because of this or you just re-checked the code?
|
An optional thought; recently I realized that |
I agree with this point, though I think we should leave it for a separate PR and discuss it in the MPHYS meeting. Does that seem reasonable? It ultimately still needs the child FFD to be added to the parent to be able to add the axis as you are suggesting, so it would rely on Josh's fix here. |
|
Agree! we can talk about it later. |
|
@joanibal: @bernardopacini and I just discussed about this PR and figured that this is probably not the best approach to address the MPhys issues. The fix in #144 already makes sense and covers MPhys problems. |
added error message for adding ref axis to grand children after the child is added to the parent
Purpose
This PR addresses some challenges @bernardopacini was having when using child FFDs with MPhys.
Expected time until merged
3 days (@bernardopacini is waiting on this)
Type of change
Testing
I ran the tandem wing optimization tutorial problem, which has 2 child FFDs. I added a ref axis before adding one the parent and the other after. The results of the optimization are exactly the same.
Checklist
flake8andblackto make sure the code adheres to PEP-8 and is consistently formatted