mdolab / pygeo

pyGeo provides geometric design variables and constraints suitable for gradient-based optimization.
https://mdolab-pygeo.readthedocs-hosted.com/en/latest/?badge=latest
Apache License 2.0
122 stars 54 forks source link

Add names to child DVGeos #201

Closed anilyil closed 9 months ago

anilyil commented 1 year ago

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

Testing

Checklist

anilyil commented 12 months ago

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 commented 12 months ago

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[bot] commented 12 months ago

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%) :arrow_up:

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

anilyil commented 11 months ago

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 commented 11 months ago

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 commented 10 months ago

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

anilyil commented 9 months ago

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