SVD composite DVs for VSP and ESP #170

ArshSaja · 2023-01-03T23:03:19Z

Purpose

Expected time until merged

This PR includes the @nwu63 composite DV concept of FFD cases. A similar formulation is implemented to use the SVD transformation for VSP and ESP design variables. A test for VSP is also included.

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

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

codecov · 2023-01-03T23:21:33Z

Codecov Report

Merging #170 (e3916c9) into main (4ac53e0) will decrease coverage by 0.36%.
The diff coverage is 61.02%.

@@            Coverage Diff             @@
##             main     #170      +/-   ##
==========================================
- Coverage   64.14%   63.78%   -0.36%     
==========================================
  Files          47       47              
  Lines       11866    11954      +88     
==========================================
+ Hits         7611     7625      +14     
- Misses       4255     4329      +74

Impacted Files	Coverage Δ
pygeo/mphys/mphys_dvgeo.py	`0.00% <0.00%> (ø)`
pygeo/parameterization/BaseDVGeo.py	`66.66% <61.53%> (-3.34%)`	⬇️
pygeo/parameterization/DVGeoSketch.py	`68.22% <64.17%> (-1.55%)`	⬇️
pygeo/parameterization/DVGeoESP.py	`66.29% <91.66%> (+1.41%)`	⬆️
pygeo/parameterization/DVGeoVSP.py	`81.54% <92.30%> (+0.11%)`	⬆️
pygeo/parameterization/DVGeo.py	`68.63% <100.00%> (+2.23%)`	⬆️
pygeo/constraints/areaConstraint.py	`50.50% <0.00%> (-25.42%)`	⬇️
pygeo/constraints/DVCon.py	`68.48% <0.00%> (-3.25%)`	⬇️
pygeo/parameterization/designVars.py	`79.25% <0.00%> (+1.59%)`	⬆️
... and 1 more

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

hajdik

Very nice, great that the SVD DVs can be used for other parameterizations now! I have some (primarily formatting) comments to start.

pygeo/mphys/mphys_dvgeo.py

pygeo/parameterization/DVGeo.py

pygeo/parameterization/DVGeoESP.py

pygeo/parameterization/DVGeoSketch.py

ewu63

You should add a test for the FFD stuff, and make sure the refactoring does not change the behaviour of that code.

ArshSaja · 2023-01-11T02:33:26Z

Why was this argument added? It looks like s gets set to none in one case and returned from a function in the other case so the argument is always overwritten @hajdik

Right! the option was not necessary, it was removed.

hajdik · 2023-01-11T14:26:45Z

When the composite DVs are added to openmdao, the default VSP DVs are DVs anymore. Therefore, we don't have output for these DVs. If the VSP DVs are added as inputs, OpenMDAO will consider them as DVs.

So does this not happen for other composite DVs when used in MPhys?

ArshSaja · 2023-01-11T15:46:55Z

When the composite DVs are added to openmdao, the default VSP DVs are DVs anymore. Therefore, we don't have output for these DVs. If the VSP DVs are added as inputs, OpenMDAO will consider them as DVs.

So does this not happen for other composite DVs when used in MPhys?

This is the same for all composite DV cases. The optimizer has the information on composite DVs only, it doesn't have the information on the default DVs from VSP. pyGeo does the transformation from composite to default DVs when the necessary changes are required inside it.

hajdik · 2023-01-11T15:57:32Z

This is the same for all composite DV cases. The optimizer has the information on composite DVs only, it doesn't have the information on the default DVs from VSP. pyGeo does the transformation from composite to default DVs when the necessary changes are required inside it.

If it would be the same for all composite DVs, shouldn't the if add_input statement be added to all of the nom_add functions so that MPhys will work with all DVGeos and composite DVs? Also, could you add a comment explaining why the if statement is there to the code so it is more clear?

ArshSaja · 2023-01-11T16:16:52Z

This is the same for all composite DV cases. The optimizer has the information on composite DVs only, it doesn't have the information on the default DVs from VSP. pyGeo does the transformation from composite to default DVs when the necessary changes are required inside it.

If it would be the same for all composite DVs, shouldn't the if add_input statement be added to all of the nom_add functions so that MPhys will work with all DVGeos and composite DVs? Also, could you add a comment explaining why the if statement is there to the code so it is more clear?

Added the composite optionality to all DVGeo.

hajdik · 2023-01-17T22:25:20Z

pygeo/mphys/mphys_dvgeo.py

        # global DVs are only added to FFD-based DVGeo objects
        if self.geo_type != "ffd":
            raise RuntimeError(f"Only FFD-based DVGeo objects can use global DVs, not type:{self.geo_type}")

        # define the input
-        self.add_input(dvName, distributed=False, shape=len(value))
+        # When composite DVs are used, input is not required for the default DVs
+        if add_input:


I still think that the reason this step happens is unclear when you just read the code and don't have the context of the PR. Because add_input is used IFF the DV is composite (correct me if I'm wrong), I think it would make more sense to have the variable name reflect that. Like if the variable name were isComposite or something similar so the check becomes
if isComposite(): <do thing>
and then the comment can say that the initial setup of the composite DV makes the extra step necessary.

ArshSaja · 2023-01-18T16:05:13Z

The add_input is not required when we use composite DVs. so if isComposite is false by default, the input has to be added to the OpenMDAO. If isComposite is true, then we don't need to add the inputs. Now the name convention should be fine.

ewu63 · 2023-01-21T18:54:18Z

Have we verified that the FFD DVGeo SVD tests also pass on the main branch before the PR?

ArshSaja · 2023-01-21T18:56:24Z

Yes I tested the same test with main branch. It was working.

ewu63

I don't have time to review this PR in detail, it would be good if someone else can take a look.

ArshSaja added 18 commits August 31, 2022 16:36

adding svd

b2341ac

adding svd work

f02bc7c

adding svd work

96ff85b

changes are done

7f33347

Fixed DV name

e88f999

Fixed the Dv name in addVarGroup

baae253

python ordered dict fix

d008ce1

fix array

206e0a4

fix array

53f074c

Minor Fixes

6239eb9

column vector fix

6b6ba21

column vector fix

dca64ad

column vector fix

59556b8

column vector fix

9bdd6ac

merging main

1ccb650

Merge branch 'main' of github.com:ArshSaja/pygeo into main

5512246

merging main

d90c59a

adding tests for OpenVSP composite DV

4cbdea6

ArshSaja requested review from ewu63 and hajdik January 3, 2023 23:03

ArshSaja requested a review from a team as a code owner January 3, 2023 23:03

ArshSaja requested a review from anilyil January 3, 2023 23:03

hajdik reviewed Jan 6, 2023

View reviewed changes

ArshSaja added 3 commits January 6, 2023 18:37

adding tests for ESP

3d56821

fixing code duplication and ove the identical function to basedvgeo

d93a595

abstractmethod is fixed

3fea5bf

ewu63 requested changes Jan 9, 2023

View reviewed changes

ArshSaja added 2 commits January 9, 2023 14:00

moving addcompostieDV to DVGeoSketch

408b748

adding tests for DVGeo

7224a7e

Merge branch 'main' into SVDNEW

d3bcb78

ArshSaja added 2 commits January 11, 2023 11:15

adding composite option to all DVGeo

c967d00

Merge branch 'SVDNEW' of github.com:ArshSaja/pygeo into SVDNEW

8bff09d

hajdik previously approved these changes Jan 11, 2023

View reviewed changes

minor fix to work composite in mphys

081bdac

ArshSaja dismissed hajdik’s stale review via 081bdac January 11, 2023 20:58

hajdik reviewed Jan 17, 2023

View reviewed changes

minor changes

3e6680c

minor changes

e3916c9

hajdik approved these changes Jan 18, 2023

View reviewed changes

ewu63 reviewed Jan 21, 2023

View reviewed changes

ewu63 requested review from ewu63 and removed request for ewu63 January 21, 2023 19:07

ewu63 approved these changes Jan 21, 2023

View reviewed changes

ewu63 requested review from ewu63 and removed request for ewu63 January 21, 2023 19:09

hajdik merged commit 0e67ef4 into mdolab:main Jan 23, 2023

sseraj mentioned this pull request Jan 24, 2023

Fixed test skipping #177

Merged

13 tasks

eirikurj mentioned this pull request Jan 24, 2023

Tests fail due to skipped tests #178

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SVD composite DVs for VSP and ESP #170

SVD composite DVs for VSP and ESP #170

ArshSaja commented Jan 3, 2023

codecov bot commented Jan 3, 2023 •

edited

Loading

hajdik left a comment

ewu63 left a comment

ArshSaja commented Jan 11, 2023

hajdik commented Jan 11, 2023

ArshSaja commented Jan 11, 2023

hajdik commented Jan 11, 2023

ArshSaja commented Jan 11, 2023

hajdik Jan 17, 2023 •

edited

Loading

ArshSaja commented Jan 18, 2023

ewu63 commented Jan 21, 2023

ArshSaja commented Jan 21, 2023

ewu63 left a comment

SVD composite DVs for VSP and ESP #170

SVD composite DVs for VSP and ESP #170

Conversation

ArshSaja commented Jan 3, 2023

Purpose

Expected time until merged

Type of change

Testing

Checklist

codecov bot commented Jan 3, 2023 • edited Loading

Codecov Report

hajdik left a comment

Choose a reason for hiding this comment

ewu63 left a comment

Choose a reason for hiding this comment

ArshSaja commented Jan 11, 2023

hajdik commented Jan 11, 2023

ArshSaja commented Jan 11, 2023

hajdik commented Jan 11, 2023

ArshSaja commented Jan 11, 2023

hajdik Jan 17, 2023 • edited Loading

Choose a reason for hiding this comment

ArshSaja commented Jan 18, 2023

ewu63 commented Jan 21, 2023

ArshSaja commented Jan 21, 2023

ewu63 left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 3, 2023 •

edited

Loading

hajdik Jan 17, 2023 •

edited

Loading