timryanb commented 2 years ago

Purpose

Slight modification to DVGeoVSP to allow for projection of points within the volume of a VSP body. This should be more general than the original surface projection method and will allow for geometries with internal members (like structural models) to be parametrized using DVGeoVSP. With this PR, the more general volume projection procedure will become the new default procedure for DVGeoVSP.

Expected time until merged

a week

Type of change

[x] New feature (non-breaking change which adds functionality)

Testing

Any previously working DVGeoVSP model should be sufficient to verify that things are working. Updated the previous DVGeoVSP regression test to work with this new formulation.

Checklist

[x] I have run flake8 and black to make sure the code adheres to PEP-8 and is consistently formatted
[x] 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[bot] commented 2 years ago

Codecov Report

Merging #145 (a5c7742) into main (65bd5cf) will decrease coverage by 10.42%. The diff coverage is 77.55%.

@@             Coverage Diff             @@
##             main     #145       +/-   ##
===========================================
- Coverage   63.65%   53.23%   -10.43%     
===========================================
  Files          47       47               
  Lines       11705    11724       +19     
===========================================
- Hits         7451     6241     -1210     
- Misses       4254     5483     +1229

Impacted Files	Coverage Δ
pygeo/mphys/mphys_dvgeo.py	`0.00% <0.00%> (ø)`
pygeo/parameterization/DVGeoVSP.py	`81.43% <82.60%> (+0.03%)`	:arrow_up:
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

anilyil commented 2 years ago

We need to update the docker images for CI to work because the volume projection requires the latest VSP version. I will create a PR for this.

anilyil commented 2 years ago

I am working on updating the docker images to test this PR now. will keep you posted

timryanb commented 2 years ago

The changes look good. I only have one comment: The changes between lines 250 and 259 are a bit confusing to me. I did not really understand the logic behind how sg is set. Also, the tg is set to 0.5. Does this cause any performance loss with purely surface based geometries?

In general, my concerns raise from not understanding how the volume parameterization is done. It looks like the code does work. If that projection initial guess does not affect performance too much, I am fine with merging it. Even if it does affect performance, there's a failsafe projection w/o the initial guess so that is fine.

@timryanb, it would help a bit if you can include some comments on how the volume parameterization is done for closed volumes and for purely surface based geometries, and how these relate to the projection and the provided initial guesses. Like I said, the code changes are probably fine but a bit more explanation would be nice.

Regarding testing this on the docker images, we have the PR up, but we ended up needing additional changes to the u18 images. @nwu63 was planning on moving the u18 images to u22, so we did not look into updating the cmake version on the u18 images. If it looks like the u22 migration will take longer, we will try to update the cmake version. The latest VSP installed fine on the u20 images.

Once the tests pass with the new image, and we have a bit of explanation of the volume projection, I am fine with merging the PR. Thanks for the work @timryanb!

@anilyil, updated the code with more comments explaining volume projection procedure.

From my experience the performance from going from the surface to volume projection procedure has been negligible in compute time and equivalent in robustness. @lamkina, has also confirmed this on his engine cases.

hajdik commented 2 years ago

I think the new tests haven't ran yet because of a merge conflict in DVGeoVSP.py, once that is resolved the tests on the new docker image should run.

mdolab / pygeo

Extending DVGeoVSP to work with internal volume points #145