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

Multi-DVGeo capability in MPhys #207

Merged
merged 55 commits into from
Aug 4, 2023
Merged

Conversation

hajdik
Copy link
Contributor

@hajdik hajdik commented Jul 11, 2023

Purpose

This adds the ability to add multiple DVGeo objects to one MPhys geometry component when setting up the OM_DVGEOCOMP while preserving backward compatibility for the more common single DVGeo case.
DVGeometry classes were also modified to standardize name and type attributes to enable tracking of multiple DVGeos.

Expected time until merged

1 week

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

In addition to the MACH pyGeo tests passing, I have some test scripts for single-DVGeo and multi-DVGeo MPhys in place of actual MPhys tests.

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

@hajdik hajdik requested a review from a team as a code owner July 11, 2023 18:04
@hajdik hajdik requested review from anilyil and ArshSaja July 11, 2023 18:04
@codecov
Copy link

codecov bot commented Jul 12, 2023

Codecov Report

Merging #207 (ff37ec2) into main (86e8e9b) will decrease coverage by 0.12%.
The diff coverage is 7.03%.

@@            Coverage Diff             @@
##             main     #207      +/-   ##
==========================================
- Coverage   64.76%   64.65%   -0.12%     
==========================================
  Files          47       47              
  Lines       12018    12040      +22     
==========================================
  Hits         7784     7784              
- Misses       4234     4256      +22     
Files Changed Coverage Δ
pygeo/constraints/DVCon.py 71.87% <ø> (ø)
pygeo/constraints/areaConstraint.py 75.91% <0.00%> (ø)
pygeo/mphys/mphys_dvgeo.py 0.00% <0.00%> (ø)
pygeo/parameterization/BaseDVGeo.py 67.16% <100.00%> (+0.49%) ⬆️
pygeo/parameterization/DVGeo.py 68.76% <100.00%> (-0.02%) ⬇️
pygeo/parameterization/DVGeoCST.py 89.34% <100.00%> (ø)
pygeo/parameterization/DVGeoESP.py 66.57% <100.00%> (ø)
pygeo/parameterization/DVGeoSketch.py 67.92% <100.00%> (ø)
pygeo/parameterization/DVGeoVSP.py 81.97% <100.00%> (ø)

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

@anilyil
Copy link
Contributor

anilyil commented Jul 14, 2023

The changes are good with me, but let's have the actual users review this PR. @bernardopacini @lamkina @ArshSaja

Maybe one simplifying pattern could be to always initialize the dvgeos as if you have multiple, but if dvgeo info is not passed, you can just populate that with the single dvgeo's information. Later on in the code, instead of looping over the keys, you can just take the first entry etc. But the way its currently coded is easier to understand; just more lines of code. Either way is fine with me.

pygeo/mphys/mphys_dvgeo.py Outdated Show resolved Hide resolved
pygeo/mphys/mphys_dvgeo.py Outdated Show resolved Hide resolved
@hajdik
Copy link
Contributor Author

hajdik commented Jul 17, 2023

Maybe one simplifying pattern could be to always initialize the dvgeos as if you have multiple, but if dvgeo info is not passed, you can just populate that with the single dvgeo's information.

I implemented a version of this and tested it out, let me know what you think @anilyil

@ArshSaja
Copy link
Contributor

ArshSaja commented Jul 21, 2023

My derivatives are fine. The only thing is that if I need to get some values from the DVGeo in my runscript, I would just use self.geo.DVGeo.getValues() earlier. But now I need to call it with the default DVGeo name: self.geo.DVGeos["defaultDVGeo"].getValues(). I think you should explain this in the MPhys wrapper.

@hajdik
Copy link
Contributor Author

hajdik commented Jul 24, 2023

The only thing is that if I need to get some values from the DVGeo in my runscript, I would just use self.geo.DVGeo.getValues() earlier. But now I need to call it with the default DVGeo name: self.geo.DVGeos["defaultDVGeo"].getValues().

I think if you want to call a function that isn't in the MPhys wrapper like getValues(), the best way would be to use nom_getDVGeo() to get the DVGeo object and then call any functions directly on that.

Copy link
Contributor

@lamkina lamkina left a comment

Choose a reason for hiding this comment

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

I tested this with my aeroprop case and everything looks good. LGTM.

@anilyil anilyil merged commit cc60c2d into mdolab:main Aug 4, 2023
@hajdik hajdik deleted the mphys-multiple-dvgeo branch August 4, 2023 17:27
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.

5 participants