Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed an issue in mphys builder #160

Merged
merged 2 commits into from
Oct 3, 2022
Merged

Fixed an issue in mphys builder #160

merged 2 commits into from
Oct 3, 2022

Conversation

friedenhe
Copy link
Collaborator

@friedenhe friedenhe commented Oct 2, 2022

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

  • 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

DAFoam test that used the latest pyGeo fixed worked

Checklist

  • I have run flake8 and black to make sure the Python code adheres to PEP-8 and is consistently formatted
  • I have formatted the Fortran code with fprettify or C/C++ code with clang-format as applicable
  • 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

@friedenhe friedenhe requested a review from a team as a code owner October 2, 2022 01:35
@friedenhe friedenhe requested review from joanibal and anilyil October 2, 2022 01:35
@codecov
Copy link

codecov bot commented Oct 2, 2022

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%) ⬇️
pygeo/parameterization/DVGeoMulti.py 0.39% <0.00%> (-89.38%) ⬇️
pygeo/constraints/areaConstraint.py 50.50% <0.00%> (-25.42%) ⬇️
pygeo/constraints/DVCon.py 68.04% <0.00%> (-3.69%) ⬇️
pygeo/pyBlock.py 45.37% <0.00%> (-1.65%) ⬇️
pygeo/topology.py 54.92% <0.00%> (-0.23%) ⬇️
pygeo/__init__.py 91.30% <0.00%> (+8.69%) ⬆️
pygeo/parameterization/__init__.py 88.88% <0.00%> (+11.11%) ⬆️

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

@anilyil
Copy link
Contributor

anilyil commented Oct 2, 2022

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
Copy link
Collaborator Author

friedenhe commented Oct 2, 2022

@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
Copy link
Collaborator Author

Also, it seems that comm was not an argument form pygeo/mphys builder in a previous version. See

self.DVGeo = DVGeometry(ffd_file)

@bernardopacini
Copy link
Contributor

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.

Copy link
Contributor

@anilyil anilyil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay that makes sense. I did not realize we added it in that PR. I removed the *args for flake to work. This is the same change I had to make in my PR thats currently open.

@hajdik hajdik merged commit 4641625 into mdolab:main Oct 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants