-
-
Notifications
You must be signed in to change notification settings - Fork 572
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
#2666 put t_+ back inside div, loosen tolerance for test #2758
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## develop #2758 +/- ##
========================================
Coverage 99.68% 99.68%
========================================
Files 272 272
Lines 19007 19017 +10
========================================
+ Hits 18948 18958 +10
Misses 59 59
... and 1 file with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this correct things for the algebraic surface form? Are we just accepting that there will be a some conservation errors in that case?
Yes, there will be some error in that case, but fixing this so it's at least approximately correct in all cases is a critical bug fix |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Happy to merge once the ubuntu tests pass (I think they just need to be run again). You need to add a line to the CHANGELOG as well.
I think the issue is with one of the getting started notebooks but not sure which one... |
It works locally for me so hard to debug. Can you test it locally? |
It looks like the problem is with the model options notebook. I don't know why, but when running it in this branch it gets stuck while in the develop branch it doesn't. Could it be the change in the definition of the flux affecting the heat source terms? |
It's working fine for me. Have you tried merging the develop branch and running again? |
Which OS are you using? I suspect it might be an Ubuntu issue. |
Ubuntu 20.04 via WSL |
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
What's really weird is it also fails in the latexify branch, where I didn't change the model. Made some changes, let's see how it goes |
@all-contributors, please add @DrSOKane for reviewing PRs |
Description
Fixes bug introduced that made electrolyte conservation a bit better for constant transference number, but a lot worse for non-constant
Fixes #2666
Type of change
Please add a line in the relevant section of CHANGELOG.md to document the change (include PR #) - note reverse order of PR #s. If necessary, also add to the list of breaking changes.
Key checklist:
$ pre-commit run
(see CONTRIBUTING.md for how to set this up to run automatically when committing locally, in just two lines of code)$ python run-tests.py --all
$ python run-tests.py --doctest
You can run unit and doctests together at once, using
$ python run-tests.py --quick
.Further checks: