-
Notifications
You must be signed in to change notification settings - Fork 55
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
Conversation
Codecov Report
@@ 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
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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. |
@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): BTW: line 286 of my run script looks like this: |
Also, it seems that comm was not an argument form pygeo/mphys builder in a previous version. See pygeo/pygeo/mphys/mphys_dvgeo.py Line 26 in 65bd5cf
|
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. |
There was a problem hiding this 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.
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
flake8
andblack
to make sure the Python code adheres to PEP-8 and is consistently formattedfprettify
or C/C++ code withclang-format
as applicable