Skip to content

Arbitrary reaction#730

Merged
RemDelaporteMathurin merged 39 commits into
festim-dev:fenicsxfrom
Allentro:arbitrary_reaction
May 17, 2024
Merged

Arbitrary reaction#730
RemDelaporteMathurin merged 39 commits into
festim-dev:fenicsxfrom
Allentro:arbitrary_reaction

Conversation

@Allentro
Copy link
Copy Markdown
Contributor

@Allentro Allentro commented Apr 1, 2024

Proposed changes

Enabled an arbitrary number of reactants and products. This resolves #720

I've enabled it so that The number of products can be 0 for radioactive decay (A -> None), but the number of reactants must be at least length 1.

Types of changes

  • 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)

@RemDelaporteMathurin RemDelaporteMathurin linked an issue Apr 1, 2024 that may be closed by this pull request
@Allentro Allentro marked this pull request as ready for review April 2, 2024 21:00
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.39%. Comparing base (3d6b9da) to head (f862481).
Report is 145 commits behind head on fenicsx.

Additional details and impacted files
@@             Coverage Diff             @@
##           fenicsx     #730      +/-   ##
===========================================
- Coverage    98.39%   98.39%   -0.01%     
===========================================
  Files           27       28       +1     
  Lines         1437     1554     +117     
===========================================
+ Hits          1414     1529     +115     
- Misses          23       25       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@RemDelaporteMathurin RemDelaporteMathurin self-requested a review April 3, 2024 12:58
Copy link
Copy Markdown
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.

Hi @Allentro thanks for this! This will bring more flexibility to Reactions.

I have a couple of initial comments

Comment thread examples/multi_isotope_trapping_example.py Outdated
Comment thread examples/multi_isotope_trapping_example.py Outdated
Comment thread festim/hydrogen_transport_problem.py Outdated
Comment thread festim/hydrogen_transport_problem.py Outdated
Comment thread festim/hydrogen_transport_problem.py Outdated
Comment thread festim/reaction.py Outdated
Comment thread festim/reaction.py Outdated
Comment thread festim/species.py Outdated
Comment thread festim/reaction.py Outdated
Comment thread festim/reaction.py Outdated
Allentro and others added 7 commits April 3, 2024 20:57
Co-authored-by: Rémi Delaporte-Mathurin <40028739+RemDelaporteMathurin@users.noreply.github.com>
Co-authored-by: Rémi Delaporte-Mathurin <40028739+RemDelaporteMathurin@users.noreply.github.com>
Co-authored-by: Rémi Delaporte-Mathurin <40028739+RemDelaporteMathurin@users.noreply.github.com>
Co-authored-by: Rémi Delaporte-Mathurin <40028739+RemDelaporteMathurin@users.noreply.github.com>
Co-authored-by: Rémi Delaporte-Mathurin <40028739+RemDelaporteMathurin@users.noreply.github.com>
Co-authored-by: Rémi Delaporte-Mathurin <40028739+RemDelaporteMathurin@users.noreply.github.com>
Comment thread festim/hydrogen_transport_problem.py Outdated
Comment thread festim/reaction.py
Comment thread test/test_reaction.py
Comment thread test/test_reaction.py
Copy link
Copy Markdown
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.

@Allentro here are a few suggestions:

  • make p_0 and E_p optional arguments defaulting to None
  • test that 1) if a product is given, p_0 and E_p are not None, and 2) that if no product is given, p_0 and E_p are None.

Comment thread festim/reaction.py Outdated
Comment thread festim/reaction.py Outdated
Comment thread festim/reaction.py Outdated
@RemDelaporteMathurin RemDelaporteMathurin added enhancement New feature or request fenicsx Issue that is related to the fenicsx support labels Apr 10, 2024
@RemDelaporteMathurin RemDelaporteMathurin merged commit 8d3ab7b into festim-dev:fenicsx May 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request fenicsx Issue that is related to the fenicsx support

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Arbitrary number of reactants for Reaction

2 participants