Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix evalFunctionsSens for projected area constraint #253

Merged
merged 12 commits into from
Oct 12, 2024
Merged

Conversation

A-CGray
Copy link
Member

@A-CGray A-CGray commented Sep 18, 2024

Purpose

The evalFunctionsSens method of the projected area constraint currently fails if it is called when there are no geometric design variables because only the creation of the dAp0/1/2 arrays are skipped, but the rest of the function, that uses those arrays, is not. This is fixed by simply skipping everything if nDV ==0.

Also:

  • Altered the projected area constraint to allow for arbitrary projection directions (not just x,y,z)
  • Made the sensitivity calculation marginally more efficient
  • Split the box projected area test into one test that checks that the projected areas in the x y and z direction match the expected values, and another that tests the derivatives of the projected area in a random direction. The reason is that the sensitivities of the box's projected areas in the x, y and z directions are inaccurate due to the presence of surfaces perfectly orthogonal to the projection direction
  • Added some comments in the code explaining this issue with the derivatives
  • Switched from using np.random.seed and np.random.rand to np.random.default_rng and rng.random
  • Re-trained DVConstraints tests due to new random numbers

Expected time until merged

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

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

@A-CGray A-CGray requested a review from a team as a code owner September 18, 2024 23:53
Copy link

codecov bot commented Sep 19, 2024

Codecov Report

Attention: Patch coverage is 91.83673% with 4 lines in your changes missing coverage. Please review.

Project coverage is 65.41%. Comparing base (3070c1a) to head (8d199ad).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
pygeo/constraints/DVCon.py 73.33% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #253      +/-   ##
==========================================
- Coverage   65.42%   65.41%   -0.02%     
==========================================
  Files          47       47              
  Lines       12307    12315       +8     
==========================================
+ Hits         8052     8056       +4     
- Misses       4255     4259       +4     

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

@marcomangano
Copy link
Contributor

The changes make sense, but I wonder if it is more elegant to have something like:

if nDV == 0:
      pass

@A-CGray
Copy link
Member Author

A-CGray commented Sep 23, 2024

The changes make sense, but I wonder if it is more elegant to have something like:

if nDV == 0:
      pass

I don't think it matters, this solution would be 1 line longer than what's currently there.

marcomangano
marcomangano previously approved these changes Sep 25, 2024
@A-CGray
Copy link
Member Author

A-CGray commented Oct 8, 2024

@eytanadler so glad we figured this out so now you and @hajdik can get the mphys tests working :)

eytanadler
eytanadler previously approved these changes Oct 11, 2024
Copy link
Contributor

@eytanadler eytanadler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent fixes, thank you @A-CGray (in particular for getting to the bottom of those derivatives!

tests/reg_tests/test_DVConstraints.py Outdated Show resolved Hide resolved
tests/reg_tests/test_DVConstraints.py Outdated Show resolved Hide resolved
@hajdik hajdik merged commit 191046d into main Oct 12, 2024
12 checks passed
@hajdik hajdik deleted the A-CGray-patch-1 branch October 12, 2024 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants