Skip to content

Formulation well defined#646

Merged
jhdark merged 8 commits into
festim-dev:fenicsxfrom
jhdark:formulation_well_defined
Dec 4, 2023
Merged

Formulation well defined#646
jhdark merged 8 commits into
festim-dev:fenicsxfrom
jhdark:formulation_well_defined

Conversation

@jhdark
Copy link
Copy Markdown
Collaborator

@jhdark jhdark commented Nov 16, 2023

Proposed changes

In the case of a steady-state simulation, the solver will break if every species does not have a definition in every volume subdomain within the model. This fix adds a term to the formulation for each species not defined with a given volume subdomain in a steady-state case.

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
Copy Markdown

codecov Bot commented Nov 16, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (4130f3a) 98.34% compared to head (878caf5) 98.80%.

❗ Current head 878caf5 differs from pull request most recent head 8791c4b. Consider uploading reports for the commit 8791c4b to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##           fenicsx     #646      +/-   ##
===========================================
+ Coverage    98.34%   98.80%   +0.45%     
===========================================
  Files           20       20              
  Lines          909      919      +10     
===========================================
+ Hits           894      908      +14     
+ Misses          15       11       -4     

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

Comment thread test/test_system.py Outdated
Comment thread test/test_h_transport_problem.py Outdated
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.

Just a small update in the comment and then we're good to go!
Thanks for this! 🎉

Comment on lines +594 to +599
not_defined_in_volume = self.volume_subdomains.copy()
for vol in self.volume_subdomains:
# check reactions
for reaction in self.reactions:
if vol == reaction.volume:
not_defined_in_volume.remove(vol)
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.

What happened to sources?

Do we have a test for this case:

  • 2 subdomains
  • 2 species (one mobile, one immobile)
  • 1 reaction in subdomain 1 only (involving the two species)
  • 1 source term in subdomain 2 for the immobile species only

I think this should be allowed. In the current implementation, you would apply c_2 = 0 in subdomain 2 even though you have a source term there!

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.

Oh thinking about this more, since this is only for steady state, I think that this case would be ill-posed anyway. Let's keep it like this!

Comment thread festim/hydrogen_transport_problem.py Outdated
@RemDelaporteMathurin RemDelaporteMathurin added the fenicsx Issue that is related to the fenicsx support label Nov 21, 2023
Co-authored-by: Rémi Delaporte-Mathurin <40028739+RemDelaporteMathurin@users.noreply.github.com>
@jhdark jhdark merged commit 71039f3 into festim-dev:fenicsx Dec 4, 2023
@jhdark jhdark deleted the formulation_well_defined branch December 4, 2023 10:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fenicsx Issue that is related to the fenicsx support

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants