DerivedQuantities for fenicsx

[JonathanGSDUFOUR]

Proposed changes

A first look into what I have for now in trying to work on Issue #638 .

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

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.95%. Comparing base (2f9f966) to head (dcccfc5).
Report is 49 commits behind head on fenicsx.

Additional details and impacted files

@@             Coverage Diff             @@
##           fenicsx     #735      +/-   ##
===========================================
+ Coverage    98.90%   98.95%   +0.05%     
===========================================
  Files           28       35       +7     
  Lines         1550     1631      +81     
===========================================
+ Hits          1533     1614      +81     
  Misses          17       17              

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

[RemDelaporteMathurin]

@JonathanGSDUFOUR glad to see progress on this! Have you tried testing your implementation on an example?

[JonathanGSDUFOUR]

@JonathanGSDUFOUR glad to see progress on this! Have you tried testing your implementation on an example?

Not yet, but it's a good next step indeed

[JonathanGSDUFOUR]

Would it be better to test each quantity separately or can we test them all in one case ?

[RemDelaporteMathurin]

Would it be better to test each quantity separately or can we test them all in one case ?

We want to test each quantity ensuring it computes the expected value.
A way to do it would be to create a function, store it as the species.solution and then check that the value computed by the derived quantity is the expected one.

[RemDelaporteMathurin]

@JonathanGSDUFOUR I don't know if you are aware but:

• the tests don't pass with your changes. You can run the tests locally with python -m pytest .
• there are a couple of unadressed comments

[JonathanGSDUFOUR]

@JonathanGSDUFOUR I don't know if you are aware but:

• the tests don't pass with your changes. You can run the tests locally with python -m pytest .
• there are a couple of unadressed comments

Yeah I currently have an issue with the version of dolfinx on my environment so the test don't run correctly, so I'm a bit blind on that, I wanted to share some of the new additions on the PR.
I will go back to the comment next

[RemDelaporteMathurin]

I currently have an issue with the version of dolfinx on my environment

What's the issue?

[JonathanGSDUFOUR]

I currently have an issue with the version of dolfinx on my environment

What's the issue?

I had the latest version of dolfinx in my conda env (0.8.0) but it's solved now (I have one with 0.7.3 that runs the test correctly)

[RemDelaporteMathurin]

@JonathanGSDUFOUR tests pass yay 🎉

[RemDelaporteMathurin]

This is looking very good!

• Not for this PR but I think it would be good to raise an issue to remind ourselves to write tests for the surface classes in 2D since MaximumSurface MinimumSurface AverageSurface and TotalSurface all give the exact same value in 1D.
• To test that MinimumVolume and MaximumVolume compute the right value in the right subdomain, could we write tests for a multi-material problem? I guess I have the same concerns about other classes

[RemDelaporteMathurin]

Can you remind me where we have the type check for bool here @jhdark ?

[RemDelaporteMathurin]

You can simplify this to:

Suggested change

	if (
	isinstance(export, F.SurfaceFlux)
	or isinstance(export, F.TotalSurface)
	or isinstance(export, F.AverageSurface)
	):
	if isinstance(export, (F.SurfaceFlux, F.TotalSurface, F.AverageSurface)):

[RemDelaporteMathurin]

Same comment as above

[RemDelaporteMathurin]

hey @JonathanGSDUFOUR this is good to go! Just two small refactoring comments and we can merge 🎉

[RemDelaporteMathurin]

This is all good to go @JonathanGSDUFOUR ! Thanks for this effort! 🎉