Skip to content

Conversation

@jhdark
Copy link
Collaborator

@jhdark jhdark commented Apr 4, 2025

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?

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Code refactoring
  • Documentation Update (if none of the other choices apply)
  • New tests

Checklist

  • Black formatted
  • Unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

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...

@codecov
Copy link

codecov bot commented Apr 4, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.50%. Comparing base (a02ed4b) to head (7aa0ca6).
⚠️ Report is 121 commits behind head on fenicsx.

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jhdark jhdark marked this pull request as ready for review April 4, 2025 19:50
@jhdark jhdark added Documentation fenicsx Issue that is related to the fenicsx support labels Apr 4, 2025
Copy link
Collaborator

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

Copy link
Collaborator

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

Comment on lines +236 to +241
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.
Copy link
Collaborator

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

Copy link
Collaborator

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

Copy link
Collaborator

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

Copy link
Collaborator

@RemDelaporteMathurin RemDelaporteMathurin left a 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

@jhdark jhdark merged commit 58c2c0d into festim-dev:fenicsx Aug 28, 2025
8 of 9 checks passed
@jhdark jhdark deleted the bc_docs branch August 28, 2025 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Documentation fenicsx Issue that is related to the fenicsx support

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants