-
Notifications
You must be signed in to change notification settings - Fork 32
Documentation for BC's in v2.0 #974
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## fenicsx #974 +/- ##
===========================================
- Coverage 95.56% 95.50% -0.07%
===========================================
Files 45 45
Lines 3089 3089
===========================================
- Hits 2952 2950 -2
- Misses 137 139 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
It may be worth stating somewhere that all boundary conditions need to have a SurfaceSudomain object, with a link to the Subdomain section
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 see this is explained for heat transfer so maybe not needed
| Surface reactions | ||
| ------------------ | ||
|
|
||
| Surface reactions on boundary can be defined with the :class:`festim.SurfaceReactionBC` class. | ||
|
|
||
| The surface reaction class can be used to impose dissociation and recombination reactions on the surface of the material. |
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.
Here, it is quite important to detail the mathematical description of these reactions. Either here or in the theory section because there is a slight change compared to FESTIM1.
Maybe a first usage example would be the H2 <-> 2H reaction.
Then the A + B <-> AB reaction
Then the full:
A + B <-> AB
A + A <-> A
B + B <-> BB
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 wonder if we can cover isotopic exchange like
H2O (g) + T (adsorbed) <-> HTO + H (adsorbed)
or
H2 (g) + T (ad) <-> HT (g) + H (ad)
I don't think we can atm
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.
Btw, this can cover tri-species reactions too:
ABC <-> A + B + C
Co-authored-by: Rémi Delaporte-Mathurin <40028739+RemDelaporteMathurin@users.noreply.github.com>
RemDelaporteMathurin
left a comment
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'm happy with the changes. I would just spend some time detailing the theory of the surface reactions in the Theory section
Proposed changes
Describe the big picture of your changes here to communicate to the maintainers why we should accept this pull request. If it fixes a bug or resolves a feature request, be sure to link to that issue.
Types of changes
What types of changes does your code introduce to FESTIM?
Checklist
Further comments
If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...