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

DVGeoMulti test fix #199

Closed anilyil closed 1 year ago

anilyil commented 1 year ago

Purpose

The inverse distance warping math in DVGeoMulti uses a small number "eps" to prevent some denominators from being zero or getting a wrong sign. Previously, this clipping was not done in a good way, and the eps value was selected to be too small. See the diff in this PR for the relevant code in DVGeoMulti.

In this PR, I changed the clipping to more accurately represent the computational goal, increased eps from 1e-50 to 1e-20, and retrained the tests.

This is done to address the issue discussed in this PR: https://github.com/mdolab/pygeo/pull/195

This tolerance issue is coming up on this test because some test points are defined to be exactly on the intersection curve. This is a worst-case scenario in terms of numerical accuracy and this is what the eps-based clipping was trying to prevent. Real cases with intersection curves that are not straight are very unlikely to get CFD mesh points that are exactly on the curve. As a result, this change should not affect any real life application.

Increasing eps is the main mechanism that fixes this issue for the worst-case test point. However, while I was at it, I also fixed the clipping math so that its more accurate.

Finally, with the eps value of 1e-50, the ARM and X86 architectures were getting very different answers because of the numerical instability in the algorithm. Slight changes in the updated point coordinates were causing large changes in derivatives as expected. The increased eps value fixes these and the updated points are now consistent to around 10 digits instead of 4-5 with the original eps value.

Expected time until merged

1-2 days

Type of change

Testing

Checklist

codecov[bot] commented 1 year ago

Codecov Report

Merging #199 (d83f395) into main (ba53e25) will decrease coverage by 0.01%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #199      +/-   ##
==========================================
- Coverage   64.86%   64.86%   -0.01%     
==========================================
  Files          47       47              
  Lines       11945    11944       -1     
==========================================
- Hits         7748     7747       -1     
  Misses       4197     4197              
Impacted Files Coverage Δ
pygeo/parameterization/DVGeoMulti.py 91.04% <100.00%> (-0.01%) :arrow_down:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

eirikurj commented 1 year ago

Overall looks good. @anilyil are these tests trained on amd64 or arm64? Just want to check, since we do want to favor amd64 architecture. Just let me know and I can push updated values if you want.

anilyil commented 1 year ago

@eirikurj, as we discussed in our call, the updated reference is trained with the public:u22-gcc-ompi-latest image, with x86 platform, running under emulation on my ARM based mac.

This should not cause issues because the same x86 image running on my mac was passing the tests that were failing on the arm image. The changes in this PR removes the discrepancy from the two platforms so they both pass now.