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 to allow for hotstarting with systemStepExact=False #1236

Merged
merged 5 commits into from
Oct 13, 2020

Conversation

zhang-alvin
Copy link
Contributor

@zhang-alvin zhang-alvin commented Oct 9, 2020

Mandatory Checklist

Please ensure that the following criteria are met:

  • Title of pull request describes the changes/features
  • Request at least 2 reviewers
  • If new files are being added, the files are no larger than 100kB. Post the file sizes.
  • Code coverage did not decrease. If this is a bug fix, a test should cover that bug fix. If a new feature is added, a test should be made to cover that feature.
  • New features have appropriate documentation strings (readable by sphinx)
  • Contributor has read and agreed with CONTRIBUTING.md and has added themselves to CONTRIBUTORS.md

As a general rule of thumb, try to follow PEP8 guidelines.

Description

Addresses #1235. Not setting the model time prior to proceeding with calculation leads to an infinite loop.

Thanks @cekees for the suggested fix.

Added a test within tests/TwoPhaseFlow to test for the fix. Note to keep in mind is that the test is to avoid changes to the infinite loop fix.

I tried setting a test to compare numerical results but I believe something along the lines of #1184 was causing a different behavior between the directory-level pytest call and a make test call at the root.

@codecov
Copy link

codecov bot commented Oct 9, 2020

Codecov Report

Merging #1236 into master will decrease coverage by 0.00%.
The diff coverage is 75.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1236      +/-   ##
==========================================
- Coverage   52.74%   52.74%   -0.01%     
==========================================
  Files         531      531              
  Lines      109533   109536       +3     
==========================================
  Hits        57777    57777              
- Misses      51756    51759       +3     
Impacted Files Coverage Δ
proteus/tests/TwoPhaseFlow/test_TwoPhaseFlow.py 85.48% <ø> (ø)
proteus/tests/TwoPhaseFlow/damBreak.py 89.39% <66.66%> (-1.24%) ⬇️
proteus/NumericalSolution.py 70.76% <100.00%> (+0.02%) ⬆️
proteus/tests/test_boundaryconditions.py 97.33% <0.00%> (-0.25%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 317373d...e0a84d2. Read the comment docs.

@zhang-alvin
Copy link
Contributor Author

@cekees, I think the line in NumericalSolution is causing a massive code cov decrease. I can try moving the line elsewhere or is there some simpler fix to the drop in coverage.

I think @ejtovar ran into something similar pretty recently. I forget how it was resolved.

@zhang-alvin
Copy link
Contributor Author

For reference, prior to hack/removal of test, codecov indicated -36% coverage.
Removing test yields only a -0.01% drop.
Going to turn the test back on to see if behavior is corrected.

@zhang-alvin zhang-alvin merged commit e190127 into erdc:master Oct 13, 2020
@zhang-alvin zhang-alvin deleted the fixHotstart branch October 13, 2020 18:47
zhang-alvin added a commit that referenced this pull request Oct 30, 2020
* FIX: hotstart should now work even without systemStepExact

* TST: added hotstart test for 2D dambreak case without using systemStepExact

* TST: remove numerical comparison for hotstart

* HACK: try turning off test to observe codecov behavior

* TST: turn back on the hotstart TPF test
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.

2 participants