-
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
SurfaceKinetics
docs and form
#805
SurfaceKinetics
docs and form
#805
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #805 +/- ##
=======================================
Coverage 99.55% 99.55%
=======================================
Files 61 61
Lines 2705 2705
=======================================
Hits 2693 2693
Misses 12 12 ☔ View full report in Codecov by Sentry. |
Hey I'm just looking at the signs of terms when we have a negative gradient at x=0. Then we should have The first term is negative, and the RHS is also negative. Sounds good to me! |
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 for this fix.
When we take care of #798 maybe it would be beneficial to add a short derivation showing that, at x=0, steady state, then we have
Also, if we are happy with the MMS test, it might be good to add it to the CI here?
We can discuss the MMS. Personally, I don't like that it's defined on |
@RemDelaporteMathurin, I added here (also in this repo) a new MMS. Let me know what you think of it. |
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.
Looks good to me! Nice to have a MMS case for this!
Feel free to merge
Proposed changes
From discussions with @RemDelaporteMathurin, I suppose that there are mistakes in the variational form for
SurfaceKinetics
BC and API docs.The current docs states that the BC for solute species is:
$-D \nabla c_\mathrm{m} \cdot \mathbf{n} = -\lambda_{\mathrm{IS}} \dfrac{\partial c_{\mathrm{m}}}{\partial t}- J_{\mathrm{bs}} + J_{\mathrm{sb}}.$ $\mathbf{n}$ is the outward normal vector of the boundary, at steady-state 1D, this should yield: $D\dfrac{\partial c_\mathrm{m}}{\partial x}= -J_{\mathrm{bs}}+J_{\mathrm{sb}}$ , what contradicts Pick & Sonnenberg. I assume, the correct form is:
$-D \nabla c_\mathrm{m} \cdot \mathbf{n} = \lambda_{\mathrm{IS}} \dfrac{\partial c_{\mathrm{m}}}{\partial t}+J_{\mathrm{bs}} - J_{\mathrm{sb}}.$
If
To check the implementation, here is a simple MMS test, based on the approach of @RemDelaporteMathurin
Types of changes
What types of changes does your code introduce to FESTIM?
Checklist