-
Notifications
You must be signed in to change notification settings - Fork 40
Arbitrary reaction #730
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
Merged
RemDelaporteMathurin
merged 39 commits into
festim-dev:fenicsx
from
Allentro:arbitrary_reaction
May 17, 2024
Merged
Arbitrary reaction #730
Changes from all commits
Commits
Show all changes
39 commits
Select commit
Hold shift + click to select a range
df7a834
Allowing an arbitrary product number to be present in a reaction.
Allentro f4c4903
Updating docstrings and typehints
Allentro 0001121
Enabling ability for an arbitrary number of reactants.
Allentro 6f5457d
Raise ValueError if len(reactant)=0
Allentro ad3b1b4
Updating doc-strings and type hints
Allentro 9514d61
Updating current tests
Allentro 6d40e35
Updating the example files.
Allentro 0ba493d
Added tests for A -> None reactions
Allentro d75f3f5
Black formatting
Allentro 3c1381d
Fixing error in tests
Allentro dec7172
Fixing product iterator
Allentro a4bc1c5
Fixing failed tets
Allentro 7bf193e
Updating call to reaction in species
Allentro 47da437
Black formatting
Allentro 82a4784
Prevent formulation of ImplicitSpecies
Allentro ecd905a
Updating type hints of Reaction class
Allentro bdf4f12
Updating typehint or reactant input of Reaction class
Allentro fe65d63
Fixing product in test function.
Allentro c5bf901
Update examples/multi_isotope_trapping_example.py
Allentro 1567668
Update examples/multi_isotope_trapping_example.py
Allentro 875a6d1
Update festim/hydrogen_transport_problem.py
Allentro a6afaec
Update festim/hydrogen_transport_problem.py
Allentro 6a5397f
Update festim/species.py
Allentro 42a899a
Default self.product = product or []
Allentro e540546
Update festim/reaction.py
Allentro e958619
Removing superfluous list conversion
Allentro c2f8d5c
Removing superfluous conversion to list
Allentro f9491bd
Removing check for reactant being a list
Allentro f5e2fae
Removing None when no products are present
Allentro de7ac34
Adding a test against reactant=[]
Allentro b094043
Merging with upstream edits
Allentro 6f5fb29
Checking E_p and p_0 are zero with no products
Allentro f0978e3
Removing None from str() and repr() when no product is present.
Allentro daa8c5c
Fix to test
Allentro 07879fb
Test fix
Allentro 1c62b57
Fixing tests
Allentro bbd1ceb
Fixing tests
Allentro 5caf272
Fixing reactant
Allentro f862481
Adding reaction product tests
Allentro File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
If
productis optional, shouldn'tp_0andE_pbe optional too?And maybe we can raise an error if no products are given but p_0 and E_p are given and vice versa.
What do you think?
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.
Good point.
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 think an implementation that makes sense would be allowing
A -> None
In that case, k_0 and E_k are None (optional)
If users provide values for k_0 and E_k, an error is raised ("you provided a k but you didn't provide a product)
I don't think it makes sense to require users to set k=0 explicitely in this case
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.
k_0 and E_k are the forward components. Is it not the case that if product=None, p_0 and E_p should both be 0?
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.
Sorry, in all I said replace k by p (the backward rate is None if there are no products)
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.
Okay makes sense. I've added that into the PR with tests.
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 seems like p_0 and E_p are still not optional.
I would rather have users though
Reaction(reactant=..., k_0=..., E_k=...)
Than
Reaction(reactant=..., k_0=..., E_k=..., p_0=0, E_p=0)
It's easier and more straightforward for the same output.