-
Notifications
You must be signed in to change notification settings - Fork 24
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 SurfaceFlux with Soret #831
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #831 +/- ##
=======================================
Coverage 99.55% 99.55%
=======================================
Files 61 61
Lines 2718 2724 +6
=======================================
+ Hits 2706 2712 +6
Misses 12 12 ☔ 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.
The fix seems ok.
I have a couple of questions below.
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.
I suppose everything is ok.
Proposed changes
This is a fix for #830.
The
compute
method ofSurfaceFlux
had asoret
argument defaulting toFalse
. However, this was not integrated correctly with the rest of the implementation.What I did is I removed this argument and replaced it by an attribute of
SurfaceFlux
. This attribute is set toTrue
orFalse
duringSimulation.initialise
. Also, theT
attribute ofSurfaceFlux
is set at the same time in order to calculate the soret flux.This bug did not affect any of the actual solving step and the equations were correctly solved with Soret. This is only a post-processing bug with SurfaceFlux (the other derived quantities are unaffected).
Note: I propagated this fix to the cylindrical and spherical versions of
SurfaceFlux
.Types of changes
What types of changes does your code introduce to FESTIM?
Checklist