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

Multiple BC Fix #354

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Multiple BC Fix #354

wants to merge 12 commits into from

Conversation

lamkina
Copy link
Contributor

@lamkina lamkina commented May 14, 2024

Purpose

This PR addresses a bug where adding multiple BCs of the same type didn't work as expected. Previously, only the first BC of a given type would be properly added and tracked, but then any subsequent BC of the same type would be ignored. This resulted in incorrect functionals and derivatives with zero seeds.

Using the suggested fixes from PR #323 this should now be fixed. I added a new mesh to input_files and tests to make sure this is working correctly.

Expected time until merged

1-3 weeks

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

Run testflo tests/reg_tests/test_multiple_bc.py to test the changes. You can also run the complex tests in the same file to verify the derivatives are working as expected.

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

@lamkina lamkina requested review from eirikurj and sseraj May 14, 2024 14:56
@lamkina lamkina requested a review from a team as a code owner May 14, 2024 14:56
Copy link

codecov bot commented May 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 27.57%. Comparing base (20ff867) to head (0ebd9ac).

❗ There is a different number of reports uploaded between BASE (20ff867) and HEAD (0ebd9ac). Click for more details.

HEAD has 5 uploads less than BASE
Flag BASE (20ff867) HEAD (0ebd9ac)
8 3
Additional details and impacted files
@@             Coverage Diff             @@
##             main     #354       +/-   ##
===========================================
- Coverage   41.02%   27.57%   -13.45%     
===========================================
  Files          13       13               
  Lines        4119     4119               
===========================================
- Hits         1690     1136      -554     
- Misses       2429     2983      +554     
Flag Coverage Δ
27.57% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@lamkina
Copy link
Contributor Author

lamkina commented May 14, 2024

@sseraj @eirikurj The failing tests are passing locally so I'm not sure what is happening. This specific test is just checking the totals of BC integrated functions computed with the adjoint against the totals that are saved in the training data. Locally it matches to 1e-12 absolute and relative tolerance, but on the images it's at worst getting 1e-6 absolute and 1e-5 relative.

Any debugging tips on what might be happening?

@sseraj
Copy link
Collaborator

sseraj commented May 14, 2024

I have had tests fail like this when the flow converges fine locally but does not fully converge on the Docker images. I would check that the flow and adjoint both converge to the same level on the Docker images as on your local machine. You might have to increase nCycles

@lamkina
Copy link
Contributor Author

lamkina commented Oct 8, 2024

Following the discussion in #358 we should update the patch version in this PR when it's eventually ready.

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.

3 participants