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
131 stars 55 forks source link

Make ref axis optional when adding a child FFD #150

Closed joanibal closed 2 years ago

joanibal commented 2 years ago

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

codecov[bot] commented 2 years ago

Codecov Report

Merging #150 (07a519d) into main (fd60570) will decrease coverage by 10.45%. The diff coverage is 82.35%.

@@             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     
Impacted Files Coverage Δ
pygeo/parameterization/DVGeo.py 64.23% <81.25%> (-0.42%) :arrow_down:
pygeo/__init__.py 91.30% <100.00%> (+8.69%) :arrow_up:
pygeo/parameterization/DVGeoMulti.py 0.40% <0.00%> (-89.42%) :arrow_down:
pygeo/constraints/areaConstraint.py 50.50% <0.00%> (-25.42%) :arrow_down:
pygeo/constraints/DVCon.py 68.04% <0.00%> (-3.69%) :arrow_down:
pygeo/pyBlock.py 45.37% <0.00%> (-1.65%) :arrow_down:
pygeo/topology.py 54.92% <0.00%> (-0.23%) :arrow_down:
pygeo/parameterization/__init__.py 88.88% <0.00%> (+11.11%) :arrow_up:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

ArshSaja commented 2 years ago

An optional thought; recently I realized that def nom_addRefAxis(self,comp=None, **kwargs): can be removed from mphys_dvgeo.py since it is easy to call the function from group level which is smth similar to this: self.geo.DVGeo.addRefAxis(name="wing",curve=c1,volumes=[0] )

bernardopacini commented 2 years ago

An optional thought; recently I realized that def nom_addRefAxis(self,comp=None, **kwargs): can be removed from mphys_dvgeo.py since it is easy to call the function from group level which is smth similar to this: self.geo.DVGeo.addRefAxis(name="wing",curve=c1,volumes=[0] )

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.

ArshSaja commented 2 years ago

Agree! we can talk about it later.

marcomangano commented 2 years ago

@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. The fix in this PR is alright but 1) we are adding a limitation on the number of children levels you can add (not the best practice, but I think now we can have grandchildren, grand-grandchildren and so on) and 2) I feel that instead of adding and additional patch, we should sit down, understand the full workflow, and possibly refactor the FFD initialization and update sequence.