Average surface and volume derived quantities in Cylindrical and Spherical #820

jhdark · 2024-07-26T12:09:12Z

Proposed changes

Describe the big picture of your changes here to communicate to the maintainers why we should accept this pull request. If it fixes a bug or resolves a feature request, be sure to link to that issue.

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 · 2024-07-26T12:37:11Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.57%. Comparing base (282071e) to head (e747d52).
Report is 26 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #820   +/-   ##
=======================================
  Coverage   99.56%   99.57%           
=======================================
  Files          61       61           
  Lines        2747     2796   +49     
=======================================
+ Hits         2735     2784   +49     
  Misses         12       12

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

festim/exports/derived_quantities/average_surface.py

RemDelaporteMathurin

Here are a few comments.

On a side note, this makes me think we should try and test for cases where the surface varies in both $r$ and $z$. Like this:

RemDelaporteMathurin · 2024-08-05T14:08:47Z

festim/exports/derived_quantities/average_surface.py

@@ -47,3 +48,101 @@ def compute(self):
        return f.assemble(self.function * self.ds(self.surface)) / f.assemble(
            1 * self.ds(self.surface)
        )
+
+
+class AverageSurfaceCylindrical(AverageSurface):


Since #832 you need to override the allowed_meshes property for these quantities.

Is this where we could make use of things like abstract methods and stuff?

RemDelaporteMathurin · 2024-08-05T14:18:02Z

festim/exports/derived_quantities/average_surface.py

+        # dV_z = r dr dtheta , assuming axisymmetry dV_z = theta r dr
+        # dV_r = r dz dtheta , assuming axisymmetry dV_r = theta r dz
+        # in both cases the expression with self.dx is the same


Not true for spherical, please adapt

RemDelaporteMathurin · 2024-08-05T14:20:52Z

festim/exports/derived_quantities/average_surface.py

+        return avg_surf
+
+
+class AverageSurfaceSpherical(AverageSurface):


Since spherical coordinates can only be used in 1D, then the surface is always 0D.

Do we need this class at all? Since for 0D surfaces $\int f \ dS = f$ then:

$$ \frac{\int f \ ds }{\int 1 \ ds} = \frac{f}{1} = f $$

Then we have AverageSurfaceSpherical = AverageSurface.

Does that make sense?

RemDelaporteMathurin · 2024-08-05T14:26:19Z

test/unit/test_exports/test_derived_quantities/test_average_surface.py

+    top_id = 2
+    top_surface.mark(surface_markers, top_id)
+
+    my_export = AverageSurfaceCylindrical("solute", top_id)


These tests pass if you replace the class on this line with AverageSurface (cartesian).

Maybe the test case isn't chosen correctly chosen

That's because the concentration field is constant on the top surface...

Try with

c_fun = lambda r: 2 * r

RemDelaporteMathurin · 2024-08-05T14:47:35Z

test/unit/test_exports/test_derived_quantities/test_average_surface.py

+    right_surface.mark(surface_markers, right_id)
+    ds = f.Measure("ds", domain=mesh_fenics, subdomain_data=surface_markers)
+
+    my_export = AverageSurfaceSpherical("solute", right_id)


Follwing my comment above (AverageSurfaceSpherical = AverageSurface) if you replace this by AverageSurface the tests pass

RemDelaporteMathurin · 2024-08-05T14:51:28Z

test/unit/test_exports/test_derived_quantities/test_average_volume.py

+    volume_markers.set_all(1)
+    dx = f.Measure("dx", domain=mesh_fenics, subdomain_data=volume_markers)
+
+    my_export = AverageVolumeCylindrical("solute", 1)


Same comment as above. If you change this to simply AverageVolume then the tests pass. The c_fun function doesn't vary in $r$ so you're not really testing that your implementaiton works well here

Try with $c = 3r$

RemDelaporteMathurin · 2024-08-05T14:55:59Z

test/unit/test_exports/test_derived_quantities/test_average_volume.py

+
+    my_export = AverageVolumeSpherical("solute", 1)
+    V = f.FunctionSpace(mesh_fenics, "P", 1)
+    c_fun = lambda r: c_left + (c_right - c_left) / (r1 - r0) * r


Just noting that $c$ is not equal to $c_\mathrm{right}$ with this expression (replace $r$ by $r1$)

jhdark added 8 commits July 25, 2024 23:19

first effort

fb31540

add new classes

5fb6032

remove comments and testing

cd7c930

fix

e82da3a

surface not volume

740e8e6

fix documentation

b0ce4fb

fix azimuth range spherical and doc strings

a12c519

no more than pi in range

307b95b

jhdark added 2 commits July 26, 2024 18:18

ranges cancel out surface

a83849e

remove angles

4bc4a6a

RemDelaporteMathurin added the enhancement New feature or request label Jul 29, 2024

RemDelaporteMathurin reviewed Aug 2, 2024

View reviewed changes

festim/exports/derived_quantities/average_surface.py Show resolved Hide resolved

jhdark added 8 commits August 5, 2024 09:01

Merge branch 'main' into cyl_and_sph_avg_quantities

861fb61

update doc strings

bb1a023

remove print and quit

d4fe1c2

added tests

d904cb3

rename tests

b96300d

change export name

41b90f1

test average volume

5944b68

dont need haevy mesh

e747d52

jhdark marked this pull request as ready for review August 5, 2024 14:05

RemDelaporteMathurin requested changes Aug 5, 2024

View reviewed changes

RemDelaporteMathurin linked an issue Aug 5, 2024 that may be closed by this pull request

Cylindrical and spherical versions of derived quantities #723

Open

4 tasks

RemDelaporteMathurin mentioned this pull request Aug 5, 2024

Cylindrical and spherical versions of derived quantities #723

Open

4 tasks

RemDelaporteMathurin removed a link to an issue Aug 5, 2024

Cylindrical and spherical versions of derived quantities #723

Open

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Average surface and volume derived quantities in Cylindrical and Spherical #820

Average surface and volume derived quantities in Cylindrical and Spherical #820

jhdark commented Jul 26, 2024 •

edited

Loading

codecov bot commented Jul 26, 2024 •

edited

Loading

RemDelaporteMathurin left a comment

RemDelaporteMathurin Aug 5, 2024

RemDelaporteMathurin Aug 5, 2024

RemDelaporteMathurin Aug 5, 2024

RemDelaporteMathurin Aug 5, 2024

RemDelaporteMathurin Aug 5, 2024

RemDelaporteMathurin Aug 5, 2024

RemDelaporteMathurin Aug 5, 2024

RemDelaporteMathurin Aug 5, 2024

RemDelaporteMathurin Aug 5, 2024

RemDelaporteMathurin Aug 5, 2024

		return avg_surf


		class AverageSurfaceSpherical(AverageSurface):

Average surface and volume derived quantities in Cylindrical and Spherical #820

Are you sure you want to change the base?

Average surface and volume derived quantities in Cylindrical and Spherical #820

Conversation

jhdark commented Jul 26, 2024 • edited Loading

Proposed changes

Types of changes

Checklist

Further comments

codecov bot commented Jul 26, 2024 • edited Loading

Codecov Report

RemDelaporteMathurin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhdark commented Jul 26, 2024 •

edited

Loading

codecov bot commented Jul 26, 2024 •

edited

Loading