bug fix for min/max values not working on discontinuous problem by ee-nn · Pull Request #1226 · festim-dev/FESTIM

ee-nn · 2026-08-08T21:11:30Z

Description

Provides a fix for issue #1225.

Summary

Essentially #1225 was the result of two separate problems:

initialise_exports() of HydrogenTransportProblemDiscontinuous overrides the base method from HydrogenTransportProblem, but it never initializes its own export.volume_meshtags or export.facet_meshtags. Since it doesn't invoke super() either, these attributes of export aren't carried over to the discontinuous class.
Even with the meshtags attached, compute() in Min/MaxVolume and Min/MaxSurface read self.field.post_processing_solution, which stays None in the discontinuous case because the solution lives per-submesh in species.subdomain_to_post_processing_solution[subdomain].

Motivation and Context

Type of Change

🐛 Bug fix (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 (no functional changes, no API changes)
📝 Documentation update
✅ Test update (adding missing tests or correcting existing tests)
🔧 Build/CI configuration change

Testing

All existing tests pass locally (pytest)
I have added new tests that prove my fix is effective or that my feature works

Specifically, new system test for the discontinuous case (which is essentially the MWE listed in #1225 - not sure if this is overkill?), and some smaller assertion tests to ensure that meshtags are always defined.

Code Quality Checklist

My code follows the code style of this project (Ruff formatted: ruff format .)
My code passes linting checks (ruff check .)
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas

Documentation

I have updated the documentation accordingly (if applicable)
I have added docstrings to new functions/classes following the project conventions

codecov · 2026-08-08T21:16:13Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.64%. Comparing base (d40ae63) to head (d107866).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1226      +/-   ##
==========================================
+ Coverage   95.38%   95.64%   +0.25%     
==========================================
  Files          53       53              
  Lines        4140     4202      +62     
==========================================
+ Hits         3949     4019      +70     
+ Misses        191      183       -8

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

ee-nn · 2026-08-08T21:31:59Z

+            # the extrema exports read the solution on the submesh of the volume
+            # subdomain, so check that the species is actually defined there.
+            # a field that is not a Species with a list of subdomains (a bare name,
+            # or a species generated from a trap) is skipped: it fails earlier, for
+            # unrelated reasons
+            is_extremum = isinstance(
+                export,
+                exports.MaximumVolume
+                | exports.MinimumVolume
+                | exports.MaximumSurface
+                | exports.MinimumSurface,
+            )
+            if is_extremum and isinstance(export.field, _species.Species):
+                if isinstance(export, exports.SurfaceQuantity):
+                    volume = self.surface_to_volume[export.surface]
+                    location = f"surface {export.surface.id}"
+                else:
+                    volume = export.volume
+                    location = f"volume {volume.id}"
+                subdomains = export.field.subdomains
+                if isinstance(subdomains, list) and volume not in subdomains:
+                    raise ValueError(
+                        f"Cannot compute {export.title}: species "
+                        f"{export.field.name} is not defined in the volume subdomain "
+                        f"{volume.id} that {location} belongs to"
+                    )
+


This just provides some error catching with nice-looking raises so could be deleted if desired

I think it's good to catch this early

ee-nn · 2026-08-08T21:33:56Z

+    bot_min = F.MinimumVolume(
+        field=H, volume=bottom_volume, filename=f"{tmpdir}/bot_min.csv"
+    )
+    bot_max = F.MaximumVolume(field=H, volume=bottom_volume)
+    top_min = F.MinimumVolume(field=H, volume=top_volume)
+    top_max = F.MaximumVolume(field=H, volume=top_volume)
+    bot_surf_min = F.MinimumSurface(
+        field=H, surface=bottom_surface, filename=f"{tmpdir}/bot_surf_min.csv"
+    )
+    bot_surf_max = F.MaximumSurface(field=H, surface=bottom_surface)
+    top_surf_min = F.MinimumSurface(field=H, surface=top_surface)
+    top_surf_max = F.MaximumSurface(field=H, surface=top_surface)


Might be a little redundant, perhaps only the bottom domain could be checked?

I think it's ok to look out for everything

RemDelaporteMathurin

Thanks for this @ee-nn !!! A first pass of comments before I go more in depth

RemDelaporteMathurin · 2026-08-08T23:33:37Z

+        meshtags = self.facet_meshtags if facet_meshtags is None else facet_meshtags
+
+        if meshtags is None:
+            raise ValueError(


Could be an assertion instead with a message if false

RemDelaporteMathurin · 2026-08-08T23:34:20Z

+        values = solution.x.array[dofs]
+
+        # a process may hold no dof of the surface at all, np.max would then raise
+        local_max = np.max(values) if values.size > 0 else -np.inf


RemDelaporteMathurin · 2026-08-08T23:52:09Z

+            # the extrema exports read the solution on the submesh of the volume
+            # subdomain, so check that the species is actually defined there.
+            # a field that is not a Species with a list of subdomains (a bare name,
+            # or a species generated from a trap) is skipped: it fails earlier, for
+            # unrelated reasons
+            is_extremum = isinstance(
+                export,
+                exports.MaximumVolume
+                | exports.MinimumVolume
+                | exports.MaximumSurface
+                | exports.MinimumSurface,
+            )
+            if is_extremum and isinstance(export.field, _species.Species):
+                if isinstance(export, exports.SurfaceQuantity):
+                    volume = self.surface_to_volume[export.surface]
+                    location = f"surface {export.surface.id}"
+                else:
+                    volume = export.volume
+                    location = f"volume {volume.id}"
+                subdomains = export.field.subdomains
+                if isinstance(subdomains, list) and volume not in subdomains:
+                    raise ValueError(
+                        f"Cannot compute {export.title}: species "
+                        f"{export.field.name} is not defined in the volume subdomain "
+                        f"{volume.id} that {location} belongs to"
+                    )
+


I think it's good to catch this early

RemDelaporteMathurin · 2026-08-08T23:53:13Z

+    bot_min = F.MinimumVolume(
+        field=H, volume=bottom_volume, filename=f"{tmpdir}/bot_min.csv"
+    )
+    bot_max = F.MaximumVolume(field=H, volume=bottom_volume)
+    top_min = F.MinimumVolume(field=H, volume=top_volume)
+    top_max = F.MaximumVolume(field=H, volume=top_volume)
+    bot_surf_min = F.MinimumSurface(
+        field=H, surface=bottom_surface, filename=f"{tmpdir}/bot_surf_min.csv"
+    )
+    bot_surf_max = F.MaximumSurface(field=H, surface=bottom_surface)
+    top_surf_min = F.MinimumSurface(field=H, surface=top_surface)
+    top_surf_max = F.MaximumSurface(field=H, surface=top_surface)


I think it's ok to look out for everything

RemDelaporteMathurin · 2026-08-08T23:54:53Z

+    )
+
+    with pytest.raises(ValueError, match="facet meshtags are required"):
+        my_export.compute()


If meshtags are always required why are they not a mandatory argument of compute?

Good point, I can think about this more and update

Pushed an implementation, let me know what you think

This reverts commit 6f92175.

RemDelaporteMathurin · 2026-08-11T14:12:30Z

+        u: dolfinx.fem.Function | None = None,
+        volume_meshtags: dolfinx.mesh.MeshTags | None = None,


For a given problem, one would never invoke compute() with different volume mesthags and different us right? I think they should be attributes instead and set inside problem.initialise() somewhere. (I think we have a initialise_exports method)

RemDelaporteMathurin · 2026-08-11T14:12:46Z

+                defined on. This is the expected behaviour for the discontinuous
+                problem, where each volume subdomain owns its own submesh.
+        """
+        solution = self.field.post_processing_solution if u is None else u


this could be a property

or a method rather

RemDelaporteMathurin · 2026-08-11T14:13:28Z

+                problem, where each volume subdomain owns its own submesh.
+        """
+        solution = self.field.post_processing_solution if u is None else u
+        meshtags = self.volume_meshtags if volume_meshtags is None else volume_meshtags


this should be an attribute

Does this only apply to the volume_meshtag class and then we keep the facet_meshtags argument for the surface?

i think this would apply to both Volume quantities and Surface Quantities (if that's what you're asking)

Ah yeah that's what I was getting at, thanks 🙂

RemDelaporteMathurin · 2026-08-11T14:13:55Z

+                defined on. This is the expected behaviour for the discontinuous
+                problem, where each volume subdomain owns its own submesh.
+        """
+        solution = self.field.post_processing_solution if u is None else u


at the beginning of compute we should have an assertion to make sure that meshtags have been given

This would only be needed for when we are not on a submesh right? If we have a property like self.is_submesh as you suggest for the if meshtags is None statement, then this would go in the else branch of the conditional block, no?

I guess so yes!

RemDelaporteMathurin · 2026-08-11T14:24:02Z

-        )

-        self.value = mesh.comm.allreduce(np.min(solution.x.array[dofs]), op=MPI.MIN)
+        if meshtags is None:


this should maybe be a property too like self.is_submesh.

Again, one day we'll get rid of the distinction between discontinous and non-discontinuous #1191

ee-nn added 3 commits August 8, 2026 15:20

bug fix for min/max values not working on discontinuous problem

3aa13cb

update tests

742407c

some comment tweaks

54c707f

ee-nn mentioned this pull request Aug 8, 2026

[BUG] Minimum and Maximum exports fail for the discontinuous problem #1225

Open

ee-nn commented Aug 8, 2026

View reviewed changes

RemDelaporteMathurin requested changes Aug 8, 2026

View reviewed changes

make facet_meshtags a mandatory arg for min/max surface compute

ca6a0de

ee-nn added a commit to ee-nn/FESTIM that referenced this pull request Aug 10, 2026

flesh out coverage on top of festim-dev#1226

6f92175

Merge remote-tracking branch 'origin' into 1225-min-max-export-fix

6a7de63

ee-nn requested a review from RemDelaporteMathurin August 11, 2026 02:59

ee-nn added a commit to ee-nn/FESTIM that referenced this pull request Aug 11, 2026

Revert "flesh out coverage on top of festim-dev#1226"

4263f1d

This reverts commit 6f92175.

RemDelaporteMathurin requested changes Aug 11, 2026

View reviewed changes

ee-nn mentioned this pull request Aug 11, 2026

Codim: support for derived quantities #1229

Open

15 tasks

ee-nn added 6 commits August 11, 2026 22:29

apply Remi's changes to minimum and maximum volume

24f8bc5

Update Minimum/MaximumSurface and initialize_exports

0b97b45

update compute calls + tests

ec03728

update assert strings to match

455fe0f

further test update tweaks + additional test

cadc250

new test tweak

d107866

ee-nn requested a review from RemDelaporteMathurin August 12, 2026 16:34

		u: dolfinx.fem.Function \| None = None,
		volume_meshtags: dolfinx.mesh.MeshTags \| None = None,

Uh oh!

Conversation

ee-nn commented Aug 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Summary

Motivation and Context

Type of Change

Testing

Code Quality Checklist

Documentation

Uh oh!

codecov Bot commented Aug 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RemDelaporteMathurin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ee-nn commented Aug 8, 2026 •

edited

Loading

codecov Bot commented Aug 8, 2026 •

edited

Loading