Skip to content

Reaction re-work#1181

Open
jhdark wants to merge 20 commits into
festim-dev:mainfrom
jhdark:reaction_rework
Open

Reaction re-work#1181
jhdark wants to merge 20 commits into
festim-dev:mainfrom
jhdark:reaction_rework

Conversation

@jhdark

@jhdark jhdark commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

Description

Summary

Related Issues

Motivation and Context

Type of Change

  • 🐛 Bug fix (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 (no functional changes, no API changes)
  • 📝 Documentation update
  • ✅ Test update (adding missing tests or correcting existing tests)
  • 🔧 Build/CI configuration change

Testing

  • All existing tests pass locally (pytest)
  • I have added new tests that prove my fix is effective or that my feature works

Code Quality Checklist

  • My code follows the code style of this project (Ruff formatted: ruff format .)
  • My code passes linting checks (ruff check .)
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas

Documentation

  • I have updated the documentation accordingly (if applicable)
  • I have added docstrings to new functions/classes following the project conventions

Breaking Changes

Screenshots/Examples

Additional Notes

@codecov

codecov Bot commented Jul 9, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.38710% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 95.20%. Comparing base (71e8781) to head (e63f7a9).

Files with missing lines Patch % Lines
src/festim/reaction.py 98.05% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1181      +/-   ##
==========================================
+ Coverage   95.19%   95.20%   +0.01%     
==========================================
  Files          53       53              
  Lines        3951     4045      +94     
==========================================
+ Hits         3761     3851      +90     
- Misses        190      194       +4     

☔ View full report in Codecov by Harness.
📢 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.

@RemDelaporteMathurin

Copy link
Copy Markdown
Collaborator

This should make #1123 easier!

Comment thread src/festim/reaction.py
return self.p_0
else:
return 0

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add a deprecation warning here

jhdark added 12 commits July 21, 2026 16:27
- Introduced a new skill for scaffolding FESTIM components, including templates for surface-integrated derived quantities and flux boundary conditions.
- Created a review branch skill to facilitate the review process against the main branch, ensuring adherence to style rules and completeness of tests and documentation.
- Added a verification case skill to scaffold MMS or analytical verification cases, including detailed steps for deriving source terms and checking convergence orders.
- Implemented MMS patterns for system tests, providing templates for steady-state and transient cases, as well as handling multi-species and other geometries.
- Updated CLAUDE.md with comprehensive guidelines on the FESTIM codebase, including layout, environment setup, commands, target conventions, and roadmap for in-flight reworks.
- Added minimal working examples (MWE) and corresponding tests to demonstrate the usage of species and reactions within the FESTIM framework.
Comment thread src/festim/reaction.py

.. math::

R = k_1 \\prod_i c_i^{\\text{reactant}} - k_2 \\prod_j c_j^{\\text{product}}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ck768 is this form enough to account for hydride?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I don't think the HNGD equations are mass-action rates. Dissolution and nucleation are related to how far the concentration is out of equilibrium, and nucleation is super non-linear using JMAK kinetics

Source (nucleation, dissolution) ~ K*(C - equilibrium)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this a source or a reaction? the difference being that there are reactants/products in a reaction

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ck768 with this PR, the reaction rates (k) can be whatever function of temperature, space, time, and concentration we want!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah gotcha, that makes sense. So if the transport equation (for example dissolution) is:

dc_mobile/dt = K_dissolution * (c_mobile - threshold_dissolution),

this generic reaction class would K_dissolution, which depends on some arbitrary values/temp/etc/?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes K_dissolution can be anything but the form of the equation is still a bit rigid.

here the dissolution "reaction" is: hydride -> mobile
but the rate/source is not K_dissolution * c_hydride right?

Comment thread src/festim/reaction.py
Comment thread src/festim/reaction.py
Comment thread src/festim/reaction.py
Comment thread src/festim/hydrogen_transport_problem.py Outdated
Comment thread src/festim/hydrogen_transport_problem.py Outdated
Comment thread test/system_tests/test_1_mobile_1_trap.py
@jhdark
jhdark marked this pull request as ready for review July 25, 2026 19:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants