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.89%. Comparing base (4746e2d) to head (93773dc).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1226      +/-   ##
==========================================
+ Coverage   95.70%   95.89%   +0.19%     
==========================================
  Files          57       57              
  Lines        4955     5017      +62     
==========================================
+ Hits         4742     4811      +69     
+ Misses        213      206       -7

Flag	Coverage Δ
dolfinx-nightly	`95.63% <100.00%> (+0.20%)`	⬆️
dolfinx-v0.10.0	`92.92% <100.00%> (+0.22%)`	⬆️
dolfinx-v0.11.0	`95.77% <100.00%> (+0.19%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ 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-09-02T20:21:02Z

Let's merge the stack #1216 first and then fix any conflicts here

ee-nn · 2026-09-05T01:53:25Z

@RemDelaporteMathurin Ok I think this should be good to merge now

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

Closed

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

Merged

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

Merge branch 'main' into 1225-min-max-export-fix

51ea1e8

RemDelaporteMathurin approved these changes Sep 1, 2026

View reviewed changes

RemDelaporteMathurin added the bug Something isn't working label Sep 1, 2026

Merge branch 'main' into 1225-min-max-export-fix

04d5237

ee-nn and others added 3 commits September 3, 2026 10:59

Merge branch 'main' into 1225-min-max-export-fix

3f0b651

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

5b41026

move drift term block addition (fix test)

2d578fb

ee-nn self-assigned this Sep 8, 2026

Merge branch 'festim-dev:main' into 1225-min-max-export-fix

93773dc

jhdark merged commit fdfffe8 into festim-dev:main Sep 8, 2026
11 checks passed

ee-nn deleted the 1225-min-max-export-fix branch September 8, 2026 18:37

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RemDelaporteMathurin commented Sep 2, 2026

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ee-nn commented Aug 8, 2026 •

edited

Loading

codecov Bot commented Aug 8, 2026 •

edited

Loading

ee-nn commented Sep 5, 2026 •

edited

Loading