Skip to content
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

New method to handle and convert user inputs to FEniCS objects #940

Merged
merged 42 commits into from
Feb 28, 2025

Conversation

jhdark
Copy link
Collaborator

@jhdark jhdark commented Jan 27, 2025

Statement of need

Many classes within festim have a function called either create_value_fenics or create_value which is there to convert a user given input and converts it to a relevant fenics object depending on what has been provided. However, many of these functions have a lot of code duplication. Furthermore, in an effort to allow users to access given inputs, these same classes typically have the value attribute, then the value_fenics, and in some cases, an associated fem.Expression to update time dependent values within the function at each iteration.

Proposed changes

This change introduces a new class within helpers called Value which will have one argument, input_value, given by the users and remains unformatted should the user wish to retrieve it. Then 3 further attributes: ufl_expression, fenics_interpolation_expression and fenics_object.

For the implementation, classes which have value as an argument, automatically convert the user given value to a Value class object with the given value as the input_value argument. The Value class itself then has two additional properties: time_dependent and temperature_dependent depending on the value given by the user.

The class has two functions: convert_input_value which creates one of 3 acceptable FEniCS objects: fem.Constant, ufl.core.expr.Expr or fem.Function, depending on the input given by the user. In some cases such as with sources and fluxes, callable inputs given need only be passed the temperature or time or space as needed to create a ufl expression, and so an additional optional argument: up_to_ufl_expr is provided for this case. All the functions used in this conversion function are also provided in helpers if there are cases where just the function may be needed.

For the moment I have just updated the tests for test_h_transport_problem.py and test_heat_transfer_problem.py to show that this could work for both types of problems, however I haven't delved into the multi-material classes yet to see how compatible this may be.

@RemDelaporteMathurin I know you didn't want to automatically convert user given inputs, but this way they are still retrievable, what do you think?

Also very open to changing any of the naming of functions/attributes etc.

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...

Copy link

codecov bot commented Feb 20, 2025

Codecov Report

Attention: Patch coverage is 98.52941% with 2 lines in your changes missing coverage. Please review.

Project coverage is 96.02%. Comparing base (ccf59be) to head (4e04da8).
Report is 43 commits behind head on fenicsx.

Files with missing lines Patch % Lines
src/festim/helpers.py 98.93% 1 Missing ⚠️
src/festim/source.py 94.73% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           fenicsx     #940      +/-   ##
===========================================
- Coverage    96.04%   96.02%   -0.03%     
===========================================
  Files           45       45              
  Lines         2455     2490      +35     
===========================================
+ Hits          2358     2391      +33     
- Misses          97       99       +2     

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

@jhdark jhdark marked this pull request as ready for review February 24, 2025 16:26
@RemDelaporteMathurin
Copy link
Collaborator

I'll release an alpha version before merging this if you don't mind

@jhdark jhdark merged commit 01ab7f9 into festim-dev:fenicsx Feb 28, 2025
9 checks passed
@jhdark jhdark deleted the new_value_fenics branch February 28, 2025 16:41
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.

3 participants