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

Fixed an issue in mphys builder #160

Closed friedenhe closed 1 year ago

friedenhe commented 1 year ago

Purpose

Fixed an issue in the mphys builder (the comm was set as an argument and caused an error)

Expected time until merged

ASAP, this is a bug

Type of change

Testing

DAFoam test that used the latest pyGeo fixed worked

Checklist

codecov[bot] commented 1 year ago

Codecov Report

Merging #160 (6dd79f4) into main (f6ef819) will decrease coverage by 10.79%. The diff coverage is 50.00%.

@@             Coverage Diff             @@
##             main     #160       +/-   ##
===========================================
- Coverage   63.75%   52.96%   -10.80%     
===========================================
  Files          47       47               
  Lines       11786    11786               
===========================================
- Hits         7514     6242     -1272     
- Misses       4272     5544     +1272     
Impacted Files Coverage Δ
pygeo/mphys/mphys_dvgeo.py 0.00% <0.00%> (ø)
pygeo/parameterization/DVGeo.py 64.23% <100.00%> (-1.44%) :arrow_down:
pygeo/parameterization/DVGeoMulti.py 0.39% <0.00%> (-89.38%) :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/__init__.py 91.30% <0.00%> (+8.69%) :arrow_up:
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

anilyil commented 1 year ago

Ping, can you explain the bug a bit more? I am not sure if this is a bug or expected behavior. Don't we want to use the comm for the given multipoint group for the DVGeo object for example? Furthermore, I don't even thing the FFD based DVGeo would use the comm to begin with. The comm is used in the final total sensitivity reduction, and should not be needed otherwise. I am probably okay with removing it here but I wanted to understand why this is causing issues for you.

friedenhe commented 1 year ago

@anilyil I got an "unexpected keyword argument comm" error when initializing the pygeo/mphys builder. See below. @bernardopacini did you get a similar error?

Traceback (most recent call last): File “runTests_MphysAero.py”, line 286, in prob.setup(mode=“rev”) File “/home/dafoamuser/dafoam/packages/miniconda3/lib/python3.8/site-packages/openmdao/core/problem.py”, line 1061, in setup model._setup(model_comm, mode, self._metadata) File “/home/dafoamuser/dafoam/packages/miniconda3/lib/python3.8/site-packages/openmdao/core/system.py”, line 830, in _setup self._setup_procs(self.pathname, comm, mode, self._problem_meta) File “/home/dafoamuser/dafoam/packages/miniconda3/lib/python3.8/site-packages/openmdao/core/group.py”, line 608, in _setup_procs subsys._setup_procs(subsys.pathname, sub_comm, mode, prob_meta) File “/home/dafoamuser/dafoam/packages/miniconda3/lib/python3.8/site-packages/openmdao/core/component.py”, line 192, in _setup_procs self.setup() File “/home/dafoamuser/dafoam/packages/miniconda3/lib/python3.8/site-packages/pygeo/mphys/mphys_dvgeo.py”, line 38, in setup self.DVGeo = DVGeometry(self.options[“file”], comm=self.comm, *ffd_options) File “/home/dafoamuser/dafoam/packages/miniconda3/lib/python3.8/site-packages/pygeo/parameterization/DVGeo.py”, line 133, in init self.FFD = pyBlock(“plot3d”, fileName=fileName, FFD=True, kmax=kmax, args, kwargs) File “/home/dafoamuser/dafoam/packages/miniconda3/lib/python3.8/site-packages/pygeo/pyBlock.py”, line 62, in init self._readPlot3D(fileName, FFD=FFD, kmax=kmax, kwargs) TypeError: _readPlot3D() got an unexpected keyword argument ‘comm’

BTW: line 286 of my run script looks like this: self.add_subsystem("geometry", OM_DVGEOCOMP(file="FFD/wingFFD.xyz", type="ffd"))

friedenhe commented 1 year ago

Also, it seems that comm was not an argument form pygeo/mphys builder in a previous version. See https://github.com/mdolab/pygeo/blob/65bd5cfaeaedf3764fe53c2cd56507e047e601f5/pygeo/mphys/mphys_dvgeo.py#L26

bernardopacini commented 1 year ago

Yes, the COMM argument was added by mistake when we added ESP. ESP and VSP both need the COMM argument, but FFD does not and cannot take in COMM as an option. As @friedenhe mentioned, the previous version of this builder did not have COMM to begin with.