Skip to content

Permeation test case modification#591

Merged
jhdark merged 6 commits into
festim-dev:fenicsxfrom
jhdark:permeation_test
Oct 10, 2023
Merged

Permeation test case modification#591
jhdark merged 6 commits into
festim-dev:fenicsxfrom
jhdark:permeation_test

Conversation

@jhdark
Copy link
Copy Markdown
Collaborator

@jhdark jhdark commented Oct 10, 2023

Proposed changes

Made some small changes to the test case to ensure the model works. i.e re-calling the create_solver() function to add the boundary conditions, defined after initialising, to the formulation and using the ds created in the HydrogenTransportProblem class. Furthermore, the test is compared against an analytical solution.

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 Oct 10, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (4b8c671) 89.31% compared to head (45334b8) 89.23%.

Additional details and impacted files
@@             Coverage Diff             @@
##           fenicsx     #591      +/-   ##
===========================================
- Coverage    89.31%   89.23%   -0.09%     
===========================================
  Files            5        5              
  Lines          131      130       -1     
===========================================
- Hits           117      116       -1     
  Misses          14       14              
Files Coverage Δ
festim/hydrogen_transport_problem.py 90.76% <ø> (-0.14%) ⬇️

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

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.

Looks like a needed addition to this test.

Could you add docstrings to the test?

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

Thanks for addressing the comments, just a few things to change in the test

Comment thread test/test_permeation_problem.py Outdated
Comment thread test/test_permeation_problem.py Outdated
@RemDelaporteMathurin
Copy link
Copy Markdown
Collaborator

Confirmed docs builds are failing due to a change outside of festim.

I'll try fixing it in another PR, ignore for now

@RemDelaporteMathurin
Copy link
Copy Markdown
Collaborator

Now that #592 has been merged, @jhdark you may want to rebase

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.

LGTM 🚀

@jhdark jhdark merged commit 39060c6 into festim-dev:fenicsx Oct 10, 2023
@jhdark jhdark deleted the permeation_test branch October 10, 2023 19:17
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.

2 participants