Closed anilyil closed 1 year ago
Merging #168 (6917338) into main (b6e8676) will decrease coverage by
0.74%
. The diff coverage is85.71%
.
@@ Coverage Diff @@
## main #168 +/- ##
==========================================
- Coverage 64.05% 63.30% -0.75%
==========================================
Files 47 47
Lines 11814 11866 +52
==========================================
- Hits 7567 7512 -55
- Misses 4247 4354 +107
Impacted Files | Coverage Δ | |
---|---|---|
pygeo/pyNetwork.py | 66.83% <84.61%> (+5.76%) |
:arrow_up: |
pygeo/parameterization/DVGeo.py | 66.34% <90.90%> (-0.02%) |
:arrow_down: |
pygeo/constraints/areaConstraint.py | 50.50% <0.00%> (-25.42%) |
:arrow_down: |
pygeo/constraints/DVCon.py | 68.48% <0.00%> (-3.25%) |
:arrow_down: |
:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more
Thanks a lot for the review @sseraj. I addressed all of your comments, including the update in the PR description. Please let me know if there are anything else I need to fix in here.
Sorry for my late review. I am still going through the code, but at a first "deeper" look I think that the current axis
option in addRefAxis
is too confusing. The docstring is pretty clear but I feel that we should not have a kwarg that gives you two different behaviors depending on the input being a str
or an array
.
What if we split axis
into rayaxis
and normalaxis
, where the first accepts the string and the second the numpy array? We can either throw an error if both options are passed, or decide to have the second overwriting the first one.
That type of behavior change happens elsewhere in the code, and I added type checking explicitly for this. The problem with having multiple options that doe the same thing is then it is not clear at all which one takes precedence.
I will merge this once the tests pass. I want to see them run because we changed the pyspline code in the meanwhile. Thanks to @sseraj and @marcomangano for the reviews. And codecov passes for once how about that!
Purpose
The
axis
parameter when adding a reference axis determines how the links are created between each control point and the reference axis. The previous behavior only created rays from the control points and found the closest point between the ray and the reference axis to create the link from the control point to the reference axis.This approach cannot guarantee that the links will lie on the same plane a section of the FFD cross section lies. To demonstrate this, consider a wing FFD that also has some dihedral built into it. The default axis direction is
x
so the code creates rays in the x direction from all control points. Then it finds the closest point from the ref axis to the ray. Due to the dihedral, the points on the upper surface will map slightly outboard, while the points on the lower surface will map inboard. See figure that demonstrates this default behavior:In this PR, I add the capability to create the links based on the intersection of a plane and the reference axis. The plane for each control point is defined by the control point itself and a user supplied normal. The normal is handled through the axis parameter, and the code determines behavior based on the type of the input.
With the example above, if we set the axis parameter to
np.array([0.0, 0.0, 1.0)]
, the code creates a plane for each control point with the normal pointing in the z direction. Then, the reference axis links are created using the intersection of these planes with the reference axis. As a result, the control point links lie in the same plane at each section as shown here:I also added a test for this case. I add the ref axis to one of the example rectangular FFDs under the testing files with the axis direction [0, 1, 1]. The resulting links look like this:
The code then tests the vectors that define the links.
Expected time until merged
Less than a week hopefully. This is a fairly complete feature with an associated test.
Type of change
Testing
Checklist
flake8
andblack
to make sure the Python code adheres to PEP-8 and is consistently formattedfprettify
or C/C++ code withclang-format
as applicable