Add names to child DVGeos #201

anilyil · 2023-05-17T02:59:19Z

Purpose

This PR changes how DVGeo class tracks its children. Old implementation just appended the children to a list, which is then indexed in order. The current implementation saves the child DVGeometries in the self.children dictionary. The keys of the dictionary are the names of each child DVGeo. This makes the code a lot more readable since each child DVGeo is referenced by its name.

The old functionality is maintained, and unless users were accessing the child DVGeometries in a hacky way by indexing the old list, everything should work fine. Some output names might be different, but we can also fix this behavior. The names of the added pointsets will be the same if users don't provide a name for the child DVGeo.

I will apply a similar set of changes to the mphys wrapper once #193 is merged.

Expected time until merged

2 weeks

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

marcomangano

I gave a quick pass and it looks ok to me, see my comment below. Maybe we should also add a test for activeChildren?

marcomangano · 2023-05-29T10:00:23Z

pygeo/parameterization/DVGeo.py

@@ -752,6 +752,11 @@ def coordXfer(coords, mode="fwd", applyDisplacement=True, **kwargs):
        # compNames is only needed for DVGeometryMulti, so remove it if passed
        kwargs.pop("compNames", None)

+        # check if we want a custom subset of child DVGeos


This looks like an interesting addition, could you please add a short description of the arg and intended functionality in the docstrings for addPointSet?

added docstring

anilyil · 2023-08-29T08:30:53Z

I changed how constraints are added when a childIdx was being provided to use the names. Only relevant example of this that I could find in my computer was in the private tutorial. Created a PR to update it for this change: https://github.com/mdolab/private-MACH-tutorial/pull/96

anilyil · 2023-08-29T10:51:39Z

I added tests for the new capability, @marcomangano. @hajdik and @bernardopacini, I modified the MPhys wrapper accordingly. This is backwards breaking for the runscripts, though the fix needed is minimal and the current behavior is much more human readable imo. Please take a look and let me know.

This PR is ready for review.

codecov · 2023-08-31T08:42:17Z

Codecov Report

Merging #201 (4f155ef) into main (4b82a60) will increase coverage by 0.01%.
The diff coverage is 63.75%.

@@            Coverage Diff             @@
##             main     #201      +/-   ##
==========================================
+ Coverage   65.02%   65.04%   +0.01%     
==========================================
  Files          47       47              
  Lines       12106    12112       +6     
==========================================
+ Hits         7872     7878       +6     
  Misses       4234     4234

Files	Coverage Δ
pygeo/constraints/DVCon.py	`71.70% <100.00%> (ø)`
pygeo/mphys/mphys_dvgeo.py	`0.00% <0.00%> (ø)`
pygeo/parameterization/DVGeo.py	`68.91% <76.80%> (+0.09%)`	⬆️

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

bernardopacini

The changes here look good to me, just a question regarding compatibility:

How does this affect other DVGeo variants such as DVGeoMulti or DVGeoAxi? If I am not mistaken, it should be fine because they inherit this base anyways, right?

anilyil · 2023-09-14T21:08:21Z

DVGeoMulti tracks multiple regular DVGeo objects, that can have child DVGeos. DVGeoMulti itself does not have child DVGeos attached, so this will work for that directly.

I am not sure about DVGeoAxi. I don't even know if anyone is using it.

@hajdik, you mentioned the DVGeo class has a name attribute. Should we change the approach here to rely on that name attribute?

hajdik · 2023-09-18T20:35:20Z

Should we change the approach here to rely on that name attribute?

Yes, there is a name attribute in the BaseDVGeo class so everything inheriting from that has access to it, I think it makes sense to use that since it's already there.

anilyil · 2023-10-10T14:35:02Z

@hajdik, I made the suggested change on using the name attribute. This PR is ready for another round of review

hajdik · 2023-10-25T15:02:58Z

tests/reg_tests/test_DVGeometry.py

+                dIdPt[5, 1, 2] = 1.0
+                dIdx = DVGeo.totalSensitivity(dIdPt, ptName)
+
+                handler.root_add_dict(f"dIdx_{ptName}", dIdx, rtol=1e-10, atol=1e-10)


Looks like the test is failing here with KeyError: 'child1_span1' but once that's fixed I'm good with this PR

hajdik · 2023-10-25T15:03:39Z

pygeo/parameterization/DVGeo.py

@@ -908,17 +941,28 @@ def addGlobalDV(self, dvName, value, func, lower=None, upper=None, scale=1.0, co
            Use a string for a single configuration or a list for multiple
            configurations. The default value of None implies that the design
            variable applies to *ALL* configurations.
+        prependName : bool
+            Flag to determine if self.name attribute is prepended to this DV name.


Can you add something in the docstring about why this option exists?

anilyil · 2023-11-06T06:58:44Z

@marcomangano @bernardopacini can one of you take a look at this again?

bernardopacini

Apologies for the delay, this looks good to me! Thanks for the changes.

anilyil added 5 commits March 28, 2023 16:48

changed child bookkeeping to use a dict instead of list

fbf6ad4

added active child dvgeo handling

4aed9bb

fix bug with dict keys

9bf2136

added derivatives. may not work

e7d2e23

undo shape func related changes

569cde4

anilyil requested a review from a team as a code owner May 17, 2023 02:59

anilyil requested review from marcomangano and ArshSaja May 17, 2023 02:59

anilyil changed the title ~~Children dict~~ Add names to child DVGeos May 17, 2023

anilyil added 2 commits May 16, 2023 23:01

Merge branch 'main' into children_dict

6dc522c

black

93a49d4

marcomangano reviewed May 29, 2023

View reviewed changes

anilyil and others added 4 commits July 19, 2023 14:03

Merge branch 'main' into children_dict

62d788b

Merge branch 'main' into children_dict

22d5bc4

Merge remote-tracking branch 'origin/children_dict' into children_dict

c1d0598

fixes for tests

bce1320

anilyil added 4 commits August 29, 2023 10:35

added docstring

57204dc

change mphys wrapper to use child names instead of indices

a37410f

fix for the name check

4584ce2

added tests for active child argument

a141498

anilyil requested review from hajdik and bernardopacini and removed request for ArshSaja August 29, 2023 10:50

anilyil requested a review from marcomangano August 29, 2023 10:51

Merge branch 'main' into children_dict

c38bce4

bernardopacini previously approved these changes Sep 12, 2023

View reviewed changes

anilyil added 3 commits October 10, 2023 09:57

Merge branch 'main' into children_dict

418e1bf

changed the child name to use the name attribute

871ad9a

forgot to add all files

1167b5c

anilyil dismissed bernardopacini’s stale review via 1167b5c October 10, 2023 14:28

anilyil requested a review from bernardopacini October 10, 2023 14:35

sseraj linked an issue Oct 17, 2023 that may be closed by this pull request

Update childIdx to use a string rather than just an index #34

Closed

add option to not prepend self.name to DVs

b4a0079

hajdik reviewed Oct 25, 2023

View reviewed changes

anilyil added 2 commits October 26, 2023 08:53

fix the failing test

53d0883

update docstrings

899084a

anilyil requested a review from hajdik October 26, 2023 13:08

anilyil and others added 2 commits November 5, 2023 10:16

black

5b994f0

Merge branch 'main' into children_dict

4d31222

hajdik approved these changes Nov 5, 2023

View reviewed changes

bernardopacini approved these changes Nov 13, 2023

View reviewed changes

Merge branch 'main' into children_dict

4f155ef

hajdik merged commit e47a454 into mdolab:main Nov 13, 2023
12 checks passed

anilyil deleted the children_dict branch November 13, 2023 16:55

bernardopacini mentioned this pull request Dec 11, 2023

Adding DVGeometryMulti to MPhys #230

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add names to child DVGeos #201

Add names to child DVGeos #201

anilyil commented May 17, 2023

marcomangano left a comment •

edited

Loading

marcomangano May 29, 2023

anilyil Aug 29, 2023

anilyil commented Aug 29, 2023

anilyil commented Aug 29, 2023

codecov bot commented Aug 31, 2023 •

edited

Loading

bernardopacini left a comment

anilyil commented Sep 14, 2023

hajdik commented Sep 18, 2023

anilyil commented Oct 10, 2023

hajdik Oct 25, 2023

anilyil Oct 26, 2023

hajdik Oct 25, 2023

anilyil Oct 26, 2023

anilyil commented Nov 6, 2023

bernardopacini left a comment

Add names to child DVGeos #201

Add names to child DVGeos #201

Conversation

anilyil commented May 17, 2023

Purpose

Expected time until merged

Type of change

Testing

Checklist

marcomangano left a comment • edited Loading

Choose a reason for hiding this comment

marcomangano May 29, 2023

Choose a reason for hiding this comment

anilyil Aug 29, 2023

Choose a reason for hiding this comment

anilyil commented Aug 29, 2023

anilyil commented Aug 29, 2023

codecov bot commented Aug 31, 2023 • edited Loading

Codecov Report

bernardopacini left a comment

Choose a reason for hiding this comment

anilyil commented Sep 14, 2023

hajdik commented Sep 18, 2023

anilyil commented Oct 10, 2023

hajdik Oct 25, 2023

Choose a reason for hiding this comment

anilyil Oct 26, 2023

Choose a reason for hiding this comment

hajdik Oct 25, 2023

Choose a reason for hiding this comment

anilyil Oct 26, 2023

Choose a reason for hiding this comment

anilyil commented Nov 6, 2023

bernardopacini left a comment

Choose a reason for hiding this comment

marcomangano left a comment •

edited

Loading

codecov bot commented Aug 31, 2023 •

edited

Loading