Skip to content

Reaction.product now accepts a list of Species#704

Merged
RemDelaporteMathurin merged 10 commits into
festim-dev:fenicsxfrom
RemDelaporteMathurin:reactions-more-products
Mar 19, 2024
Merged

Reaction.product now accepts a list of Species#704
RemDelaporteMathurin merged 10 commits into
festim-dev:fenicsxfrom
RemDelaporteMathurin:reactions-more-products

Conversation

@RemDelaporteMathurin
Copy link
Copy Markdown
Collaborator

@RemDelaporteMathurin RemDelaporteMathurin commented Mar 5, 2024

Proposed changes

This PR fixes #702 by allowing users to give a list of Species for the product of a Reaction.

This gives us the ability to implement reactions of the type:

A + B <-> C + D

Need to merge #703 first since the tests use some of the functionality....

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)

@RemDelaporteMathurin RemDelaporteMathurin linked an issue Mar 5, 2024 that may be closed by this pull request
@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.15%. Comparing base (be788e4) to head (1cc4673).

Additional details and impacted files
@@             Coverage Diff             @@
##           fenicsx     #704      +/-   ##
===========================================
+ Coverage    99.05%   99.15%   +0.09%     
===========================================
  Files           26       26              
  Lines         1168     1183      +15     
===========================================
+ Hits          1157     1173      +16     
+ Misses          11       10       -1     

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

@RemDelaporteMathurin RemDelaporteMathurin changed the title products now accepts a list of species Reaction.product now accepts a list of Species Mar 5, 2024
@RemDelaporteMathurin RemDelaporteMathurin marked this pull request as ready for review March 5, 2024 21:43
@RemDelaporteMathurin RemDelaporteMathurin added enhancement New feature or request fenicsx Issue that is related to the fenicsx support labels Mar 5, 2024
@RemDelaporteMathurin
Copy link
Copy Markdown
Collaborator Author

@jhdark this is now ready for review

Copy link
Copy Markdown
Collaborator

@jhdark jhdark left a comment

Choose a reason for hiding this comment

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

This looks great to me, just some small suggestions but not necessary changes

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.

Does this need its own file or can it go into the test_reaction file?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I'd rather keep it seperate for now to 1) avoid super long files 2) keep things seperated until we have a discussion on how to organise tests

Comment thread test/test_reaction.py
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.

Would it be worth adding another test where we have A -> B + C?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This kind of reaction is currently not implemented

@RemDelaporteMathurin RemDelaporteMathurin merged commit ec46ff2 into festim-dev:fenicsx Mar 19, 2024
@RemDelaporteMathurin RemDelaporteMathurin deleted the reactions-more-products branch March 19, 2024 15:51
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.

Reactions.product should accept a list

2 participants