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

Fixing Constraints for new OM Parallelism Convention #191

Merged
merged 3 commits into from
Mar 14, 2023

Conversation

bernardopacini
Copy link
Contributor

@bernardopacini bernardopacini commented Mar 9, 2023

Purpose

The new OpenMDAO serial derivative parallelism convention (v3.25.0) made how we handled constraints invalid as we had different derivative values on different procs for serial inputs. The solution for this is to make constraints serial and consistent on each proc. I changed all the constraints in the MPhys wrapper as well as the compute() and compute_jacvec_product() functions to reflect these changes.

Expected time until merged

1 week.

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

I tested this using the DAFoam MPhys aero-only and aerostructural tests. The derivatives are correct (even for TACS structural functionals) and OpenMDAO does not complain about derivative inconsistencies.

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

@bernardopacini bernardopacini added bug Something isn't working maintenance This is for maintaining the repo labels Mar 9, 2023
@bernardopacini bernardopacini self-assigned this Mar 9, 2023
@bernardopacini bernardopacini requested a review from a team as a code owner March 9, 2023 20:34
@codecov
Copy link

codecov bot commented Mar 9, 2023

Codecov Report

Merging #191 (e7a3452) into main (5b7066e) will increase coverage by 0.13%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main     #191      +/-   ##
==========================================
+ Coverage   64.75%   64.88%   +0.13%     
==========================================
  Files          47       47              
  Lines       11964    11939      -25     
==========================================
  Hits         7747     7747              
+ Misses       4217     4192      -25     
Impacted Files Coverage Δ
pygeo/mphys/mphys_dvgeo.py 0.00% <0.00%> (ø)

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

anilyil
anilyil previously approved these changes Mar 10, 2023
Copy link
Contributor

@anilyil anilyil left a comment

Choose a reason for hiding this comment

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

This looks good to me, but let's also have @joanibal try this one out before merging. Thanks for these changes.

@bernardopacini
Copy link
Contributor Author

@ArshSaja @hajdik Could you both please try running check_totals() on your MPhys cases with this branch and make sure you get correct derivatives and no warnings about mismatches on processors? You may need the latest OpenMDAO commit for this, not just the latest version.

@bernardopacini bernardopacini requested review from anilyil and ArshSaja and removed request for marcomangano March 14, 2023 17:42
Copy link
Contributor

@hajdik hajdik left a comment

Choose a reason for hiding this comment

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

It's working fine for my case with a triangulated surface constraint and a 1D thickness constraint.

@anilyil anilyil merged commit 0e6b356 into mdolab:main Mar 14, 2023
@bernardopacini bernardopacini deleted the mphys_cons_fix branch March 14, 2023 23:03
@joanibal
Copy link
Collaborator

Sorry for not checking that the change worked. I appreciate you all fixing this.
It's probably best not to wait on me for anything going forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working maintenance This is for maintaining the repo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants