Closed bernardopacini closed 2 years ago
Part of the formatting failure is with one of the regression tests (unrelated to this PR). I do not think there is an actual issue, so I am just going to ignore it with a #noqa:
statement.
./tests/reg_tests/test_DVGeometryMulti.py:103:35: B023 Function definition does not bind loop variable 'nRefAxPts'.
for i in range(1, nRefAxPts)
Merging #144 (4ed8ec9) into main (7212c82) will decrease coverage by
10.54%
. The diff coverage is3.84%
.
@@ Coverage Diff @@
## main #144 +/- ##
===========================================
- Coverage 63.74% 53.19% -10.55%
===========================================
Files 47 47
Lines 11689 11705 +16
===========================================
- Hits 7451 6227 -1224
- Misses 4238 5478 +1240
Impacted Files | Coverage Δ | |
---|---|---|
pygeo/mphys/mphys_dvgeo.py | 0.00% <0.00%> (ø) |
|
pygeo/parameterization/BaseDVGeo.py | 70.00% <100.00%> (ø) |
|
pygeo/parameterization/DVGeoMulti.py | 0.40% <0.00%> (-89.42%) |
:arrow_down: |
pygeo/constraints/areaConstraint.py | 50.50% <0.00%> (-25.42%) |
:arrow_down: |
pygeo/constraints/DVCon.py | 68.04% <0.00%> (-3.69%) |
:arrow_down: |
pygeo/pyBlock.py | 45.37% <0.00%> (-1.65%) |
:arrow_down: |
pygeo/parameterization/DVGeo.py | 64.23% <0.00%> (-0.50%) |
:arrow_down: |
pygeo/topology.py | 54.92% <0.00%> (-0.23%) |
:arrow_down: |
pygeo/__init__.py | 91.30% <0.00%> (+8.69%) |
:arrow_up: |
pygeo/parameterization/__init__.py | 88.88% <0.00%> (+11.11%) |
:arrow_up: |
:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more
The code looks good. my only comment is that wouldn't it make more sense to use a dictionary approach for the child FFDs? That way we can index them by name instead of order of addition. We can use an ordered dictionary under the hood, or sort the names alphabetically and keep track of them that way. the users can pass dictionaries with child ffd name as the key and file as the value. Let me know if you think this is useful. I am also fine with merging this PR as is and doing that change separately
The code looks good. my only comment is that wouldn't it make more sense to use a dictionary approach for the child FFDs? That way we can index them by name instead of order of addition. We can use an ordered dictionary under the hood, or sort the names alphabetically and keep track of them that way. the users can pass dictionaries with child ffd name as the key and file as the value. Let me know if you think this is useful. I am also fine with merging this PR as is and doing that change separately
That approach makes sense to me. I went with this approach because it matches how pyGeo handles children, but if the indexing is confusing we could add a dictionary. If @joanibal is on board too I'll convert it to using dictionaries instead.
Edit: The additional reason for doing this is to be able to use the childIdx=
style used for constraints when defining design variables. Since we have to set the childIdx
for constraints, a user would have to keep track of the number anyways. We could change these to all be with a dictionary, but we'd be deviating further from pyGeo.
@joanibal and I found that with a small modification to pyGeo we can actually simplify adding child FFDs in MPHYS. Hold off on merging this PR until we finalize that.
Now that PR #150 is merged, I will rework this to simplify it and push new changes. I'll change this to draft until it is ready.
I fixed the package import in BaseDVGeo.py
from:
from typing import OrderedDict
to
from collections import OrderedDict
as we discussed offline.
This is now good to go!
Purpose
Adding a method for using parent + child FFDs in MPHYS. This is one possible implementation that I have been using with MPHYS + DAFoam and works as expected, but I think we should discuss if this is the best approach for how to do this. The tricky part is in the order of operations: child FFDs cannot be added to a parent without a reference axis, so they cannot be immediately set to the MPHYS geometry object. The configure method then goes in the wrong order, so we need an explicit call to
nom_add_children()
(added in this PR) to actually add the child FFDs after the reference axes are declared but before any dependent constraints are defined. The better solution here might require more careful modifications to pyGeo, but I am not sure what other issues we might run into if we were to do that.EDIT: PR #146 broke the MPHYS wrapper that was using deprecated calls to add local and global design variables. I fixed that here, though that will require updating the MPHYS examples and tests on the MPHYS repo too.
Expected time until merged
~This PR is open to discussion -- no specific timeframe set.~ 2 weeks.
Type of change
Testing
N / A
Checklist
flake8
andblack
to make sure the code adheres to PEP-8 and is consistently formatted