Added option to project thickness contraints #238

joanibal · 2024-03-07T00:35:46Z

Purpose

This PR adds the option to use the component of the toothpick thickness aligned with the original thickness direction for the constraint value.
I added this feature to combat situations where the skew of my toothpick constraints was allowing for self intersection.

In the figure below the gray mesh is the deformed FFD, the red line is the original directions of the thickness constraints and the thick black lines are the thickness constraints. The original direction of the thickness constraint is purely in the y-axis, but it is skewd by a deformation of FFD in the x-axis. With the an unprojected thickness constraint this would result in an increased thickness value, but with this projected constraint it would not.

Expected time until merged

a week would be nice

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

See the new tests added to test_DVConstraints.py

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 · 2024-03-07T01:19:31Z

Codecov Report

Attention: Patch coverage is 68.33333% with 19 lines in your changes are missing coverage. Please review.

Project coverage is 65.46%. Comparing base (820ef71) to head (f60d9ad).

Files	Patch %	Lines
pygeo/constraints/thicknessConstraint.py	62.74%	19 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #238   +/-   ##
=======================================
  Coverage   65.45%   65.46%           
=======================================
  Files          47       47           
  Lines       12208    12265   +57     
=======================================
+ Hits         7991     8029   +38     
- Misses       4217     4236   +19

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

joanibal · 2024-03-07T14:13:06Z

This is ready for review

marcomangano · 2024-03-07T20:47:22Z

This looks really interesting, I will try and take a look at this in the next few days. Can I ask what kind of DV setup / optimization made you bump into this issue?

joanibal · 2024-03-07T22:24:53Z

Thank Marco. I'm doing some funky stuff with OpenVSP where I'm changing the shape in the x and y-axis.

lamkina · 2024-03-10T18:49:45Z

I also want to take a look at this PR this week and dig into this problem some more. I have been facing this issue with toothpicks and OpenVSP for a few years now.

marcomangano

Gave a first pass, left some comments, looking good! There's quite a bit of code duplication as per pyGeo fashion but we will address that in a future effort

marcomangano · 2024-03-19T22:00:46Z

tests/reg_tests/test_DVConstraints.py

@@ -320,6 +320,53 @@ def test_thickness1D_box(self, train=False, refDeriv=False):
                funcs["DVCon1_thickness_constraints_2"], 8.0 * np.ones(3), name="thickness_base", rtol=1e-7, atol=1e-7
            )

+    def test_projected_thickness1D_box(self, train=False, refDeriv=False):


I wonder if we could use parameterized to avoid code duplication with the previous test

Perhaps, but the skewing of the design variables (line 347 onward) is different enough that it wouldn't be trivial. If we add another thickness con it might be worth creating a common function like self.genertate_dvgo_dvcon that does some common thickness testing. But since it is only like 30 lines of duplicated code total for both tests, I think its probably fine.

marcomangano · 2024-03-19T22:01:20Z

tests/reg_tests/test_DVConstraints.py

@@ -399,6 +446,64 @@ def test_thickness2D_box(self, train=False, refDeriv=False):
                funcs["DVCon1_thickness_constraints_2"], 8.0 * np.ones(4), name="thickness_base", rtol=1e-7, atol=1e-7
            )

+    def test_projected_thickness2D_box(self, train=False, refDeriv=False):


marcomangano · 2024-03-19T22:17:42Z

pygeo/constraints/thicknessConstraint.py

+        for i in range(len(self.coords) // 2):
+            handle.write("%d %d\n" % (2 * i + 1, 2 * i + 2))
+
+        handle.write("Zone T=%s_ref_directions\n" % self.name)


I would add a comment here to explain that the projected toothpicks are saved in a different zone for visualization purposes

marcomangano · 2024-03-19T22:24:43Z

pygeo/constraints/thicknessConstraint.py

+                if self.scaled:
+                    D_b /= self.D0[i]
+
+                # d(dot(vec,n)/dvec = n


Do you mind making this derivative more explicit? There's a bracket missing but it looks like replacing eDist_b is the main step here.

I have added more comments let me know what you think

joanibal · 2024-03-22T17:49:02Z

It's ready for review again

Chill flake8 it's just white space it can't hurt you

lamkina · 2024-03-22T17:53:10Z

I have a few concerns about this change, but it's mostly related to using thickness constraints with VSP generated surfaces in general.

VSP uses Bézier curves and surfaces to represent geometry. However, Bezier are not piecewise continuous like BSplines so the surface is actually decomposed into many Bézier patches. Each patch has its own u, v parameter space that can change individually. As we deform the surfaces in directions that are not parallel to the initial thickness constraints, the u, v parameters move on the individual surfaces and our initial mapping for toothpick endpoints is skewed.

With that said, if you change the top and bottom surface patches in directions non-parallel to the initial toothpick directions, it will actually move the entire constraint. This is very easy to do with VSP parameterizations. Again, this is a symptom of the thickness constraints in general with VSP, not just specific to this PR.

However, I do think this PR is a bit of a misnomer in the sense it's not actually fixing the underlying issue with the thickness constraints. In reality, the fix for this would be doing a nonlinear solve to find the new parametric coordinates that preserve the initial Cartesian orientation of the thickness constraints. I even think this issue permeates every geometry where we apply toothpick constraints with non-parallel deformations to the geometry. Most times this deformation is small and not picked up, but with VSP it is more noticeable because each Bezier patch has it's own parameter space.

joanibal · 2024-03-22T18:22:59Z

With that said, if you change the top and bottom surface patches in directions non-perpendicular to the initial toothpick directions, it will actually move the entire constraint. This is very easy to do with VSP parameterizations. Again, this is a symptom of the thickness constraints in general with VSP, not just specific to this PR.

I actually want the entire constraint to move for my particular case so I see this as a feature. I can see why you would also want constraints that are reprojected to the UV coordinate space.

However, I do think this PR is a bit of a misnomer in the sense it's not actually fixing the underlying issue with the thickness constraints.

I understand what you are saying and made the intentional choice to take a simpler path.
If time wasn't a factor, it would be nice to have a constraint where one end is locked to a U,V coordinate and the other end is reprojected to UV space. For me this PR improves upon the issue of skewed thickness constraints enough to be worth adding to the code even if there hypothetically better means.
In regard to the name, I'm not opposed to changing it if you have another idea. Although, I'd say this using the dot product here is just projecting in a different sense.

Let me know if I haven't interpreted what you are seeing as the underlying issue correctly (a picture may help me here).

lamkina · 2024-03-22T18:36:44Z

I actually want the entire constraint to move for my particular case so I see this as a feature. I can see why you would also want constraints that are reprojected to the UV coordinate space.

I can see the utility of the constraints moving if you are changing the geometry in multiple axis directions. I don't have an issue with this in general, it's more that we don't clarify the behavior anywhere and it works differently depending on which tools created the underlying geometry.

I understand what you are saying and made the intentional choice to take a simpler path.
If time wasn't a factor, it would be nice to have a constraint where one end is locked to a U,V coordinate and the other end is reprojected to UV space. For me this PR improves upon the issue of skewed thickness constraints enough to be worth adding to the code even if there hypothetically better means.
In regard to the name, I'm not opposed to changing it if you have another idea. Although, I'd say this using the dot product here is just projecting in a different sense.

In this case, I am okay with adding the projected constraints, but I think we should add a description of the behavior to the docstrings of the toothpick constraints. We can tackle adding the feature I described above in a different PR, I didn't expect you to implement that as it's going to be involved. I just wanted to make sure you knew why these things are sliding around and skewing.

lamkina · 2024-03-22T18:38:37Z

In regard to the name, I'm not opposed to changing it if you have another idea. Although, I'd say this using the dot product here is just projecting in a different sense.

The naming scheme makes sense. In the future I think we will have a few flavors of the thickness constraints with more consistent behavior.

joanibal · 2024-03-22T18:47:11Z

it works differently depending on which tools created the underlying geometry.

I'm not following this part. If the constraints was embedded in an DVGeo FFD object and there were changes along multiple axis it should work the same.

lamkina · 2024-03-22T19:05:35Z

I'm not following this part. If the constraints was embedded in an DVGeo FFD object and there were changes along multiple axis it should work the same.

My fault, I thought you were directly changing VSP variables and forgot you mentioned you are using an FFD, in which case this works differently (but also kind of makes my point on why this is confusing).

lamkina

This looks ready to go.

marcomangano

LGTM, thank you!

Joshua Anibal added 2 commits March 6, 2024 19:18

added contraint

d768705

merged in main

7c492a4

joanibal requested a review from a team as a code owner March 7, 2024 00:35

joanibal requested review from marcomangano and sseraj March 7, 2024 00:35

Joshua Anibal added 4 commits March 6, 2024 19:43

spelling and formatting

3867987

formatting try 2

0b8011a

add to mphys wrapper

5479d8f

guess at how to do reorder imports

5c166ef

lamkina self-requested a review March 10, 2024 18:49

sseraj removed their request for review March 11, 2024 16:05

marcomangano reviewed Mar 19, 2024

View reviewed changes

Update thicknessConstraint.py

5ab4a1a

joanibal requested a review from marcomangano March 22, 2024 17:48

stupid linting

b4aee7c

Chill flake8 it's just white space it can't hurt you

formating 2

f52b1f1

lamkina approved these changes Mar 25, 2024

View reviewed changes

Merge branch 'main' into projected_thickness_cons

f60d9ad

marcomangano approved these changes Mar 25, 2024

View reviewed changes

marcomangano merged commit 9caea1a into mdolab:main Mar 25, 2024
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added option to project thickness contraints #238

Added option to project thickness contraints #238

joanibal commented Mar 7, 2024

codecov bot commented Mar 7, 2024 •

edited

Loading

joanibal commented Mar 7, 2024

marcomangano commented Mar 7, 2024

joanibal commented Mar 7, 2024 •

edited

Loading

lamkina commented Mar 10, 2024

marcomangano left a comment

marcomangano Mar 19, 2024

joanibal Mar 22, 2024 •

edited

Loading

marcomangano Mar 19, 2024

marcomangano Mar 19, 2024

joanibal Mar 22, 2024

marcomangano Mar 19, 2024

joanibal Mar 22, 2024

joanibal commented Mar 22, 2024

lamkina commented Mar 22, 2024 •

edited

Loading

joanibal commented Mar 22, 2024

lamkina commented Mar 22, 2024

lamkina commented Mar 22, 2024

joanibal commented Mar 22, 2024 •

edited

Loading

lamkina commented Mar 22, 2024

lamkina left a comment

marcomangano left a comment

Added option to project thickness contraints #238

Added option to project thickness contraints #238

Conversation

joanibal commented Mar 7, 2024

Purpose

Expected time until merged

Type of change

Testing

Checklist

codecov bot commented Mar 7, 2024 • edited Loading

Codecov Report

joanibal commented Mar 7, 2024

marcomangano commented Mar 7, 2024

joanibal commented Mar 7, 2024 • edited Loading

lamkina commented Mar 10, 2024

marcomangano left a comment

Choose a reason for hiding this comment

marcomangano Mar 19, 2024

Choose a reason for hiding this comment

joanibal Mar 22, 2024 • edited Loading

Choose a reason for hiding this comment

marcomangano Mar 19, 2024

Choose a reason for hiding this comment

marcomangano Mar 19, 2024

Choose a reason for hiding this comment

joanibal Mar 22, 2024

Choose a reason for hiding this comment

marcomangano Mar 19, 2024

Choose a reason for hiding this comment

joanibal Mar 22, 2024

Choose a reason for hiding this comment

joanibal commented Mar 22, 2024

lamkina commented Mar 22, 2024 • edited Loading

joanibal commented Mar 22, 2024

lamkina commented Mar 22, 2024

lamkina commented Mar 22, 2024

joanibal commented Mar 22, 2024 • edited Loading

lamkina commented Mar 22, 2024

lamkina left a comment

Choose a reason for hiding this comment

marcomangano left a comment

Choose a reason for hiding this comment

codecov bot commented Mar 7, 2024 •

edited

Loading

joanibal commented Mar 7, 2024 •

edited

Loading

joanibal Mar 22, 2024 •

edited

Loading

lamkina commented Mar 22, 2024 •

edited

Loading

joanibal commented Mar 22, 2024 •

edited

Loading