Add left and right surface properties to VolumeSubdomain1D by user-name-owen · Pull Request #1282 · festim-dev/FESTIM

user-name-owen · 2026-10-21T06:09:20Z

Description

Summary

For 1D problems, VolumeSubdomain1D already knows its borders, so manually
defining the left/right SurfaceSubdomain1D objects is redundant. This adds
left and right properties that build them automatically:

bulk = F.VolumeSubdomain1D(id=1, borders=[0, 1e-3], material=my_mat)
left = bulk.left    # SurfaceSubdomain1D(id=-1, x=0)
right = bulk.right  # SurfaceSubdomain1D(id=-2, x=1e-3)

Related Issues

Fixes #1280

Motivation and Context

The generated surfaces use ids -1 (left) and -2 (right), which cannot collide
with user-defined positive ids.

Remove misplaced file

codecov · 2026-10-21T10:56:12Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.07%. Comparing base (ad055a8) to head (e966933).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1282   +/-   ##
=======================================
  Coverage   96.07%   96.07%           
=======================================
  Files          57       57           
  Lines        5069     5075    +6     
=======================================
+ Hits         4870     4876    +6     
  Misses        199      199

Flag	Coverage Δ
dolfinx-v0.10.0	`93.14% <100.00%> (+<0.01%)`	⬆️
dolfinx-v0.11.0	`95.94% <100.00%> (+<0.01%)`	⬆️

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-10-23T04:57:01Z

Hi @user-name-owen , welcome to FESTIM and congrats on your first PR request (I was in your shoes not too long ago!). Thanks for adding this. I can review soon.

ee-nn

@user-name-owen thanks for this and congrats on your first PR! I've left some review comments. The biggest issue is that the current changes don't run when I test them locally.

Also, adding a few tweaks to the documentation to reflect these changes could be great.

ee-nn · 2026-10-25T15:39:53Z

+        Returns:
+            festim.SurfaceSubdomain1D: the left surface subdomain
+        """
+        return SurfaceSubdomain1D(id=-1, x=self.borders[0])


Negative integers shouldn't be used here, (-1) will index all SurfaceSubdomain objects when being used in integration terms (i.e. ds(-1) will grab all domains.) The current changes also fail outright with (-2), since DOLFINx can't accept negative ids:

import numpy as np import festim as F model = F.HydrogenTransportProblem() model.mesh = F.Mesh1D(np.linspace(0, 1e-3, 1001)) steel = F.Material(D_0=1e-7, E_D=0.2) bulk = F.VolumeSubdomain1D(1, borders=[0, 1e-3], material=steel) left = bulk.left right = bulk.right model.subdomains = [bulk, left, right] T = F.Species("T") model.species = [T] model.temperature = 600 model.boundary_conditions = [ F.FixedConcentrationBC(left, value=1e20, species=T), F.FixedConcentrationBC(right, value=0, species=T), ] model.exports = [F.SurfaceFlux(field=T, surface=right)] model.settings = F.Settings(atol=1e-10, rtol=1e-10, transient=False) model.initialise() model.run()

ValueError: Integral subdomain IDs must be non-negative.

Thanks for the review! For the ids, I'd like to propose following the same convention as _allocate_manifold_side_ids: auto-generated surfaces get id=None on creation, and ids are allocated in define_meshtags_and_measures (before facet tagging) starting from max(declared ids) + 1, so they can never collide with user-declared surfaces, interfaces or volumes. Happy to hear if you'd prefer a different scheme.

Hi @user-name-owen sorry for the delayed response on this, yes this sounds like a much better scheme! Let me know when you have made the corresponding updates.

Both fixed:

Switched to @cached_property, so bulk.left/bulk.right now always return the same object.

Auto-generated surfaces carry id=None on creation; ids are allocated in define_meshtags_and_measures above every declared id (following the _allocate_manifold_side_ids convention), so they are always non-negative and cannot clash with user-declared ones. The reproducer from your review now runs to completion.

ee-nn · 2026-10-25T15:43:35Z

        )
        self.borders = borders

+    @property


Using @property here will create a new instance of SurfaceSubdomain1D on every direct access. Therefore, if the user calls bulk.left in one instance and then tries to call it again in another, they will not be accessing the same object, even though this is the expected behavior. You can used @cached_property (from functools import cached_property) to get around this

Also fixed — see my reply in the thread above: the properties now use @cached_property.

ee-nn · 2026-10-25T15:47:30Z

    )
+
+
+def test_left_and_right_surfaces():


Test will need to updated accordingly, see above comments

Updated accordingly — the test no longer asserts specific ids, and a new test_left_and_right_surfaces_get_auto_ids checks that ids are allocated above user-declared ones after initialisation.

user-name-owen · 2026-11-08T12:27:06Z

All points are addressed and pushed:

Switched to @cached_property, so bulk.left/bulk.right now always return the same object.
Auto-generated surfaces carry id=None on creation; ids are allocated in define_meshtags_and_measures above every declared id (following the _allocate_manifold_side_ids convention), so they are always non-negative and cannot clash with user-declared ones.
Updated test_subdomains.py and added test_left_and_right_surfaces_get_auto_ids (26 tests pass locally).
Documented the new usage in docs/source/userguide/subdomains.rst.

Also updated the branch with the latest main. Ready for re-review when you have time. Thanks!

RemDelaporteMathurin · 2026-11-09T08:31:08Z

+    @cached_property
    def left(self):
        """The left surface of the 1D volume subdomain.

        Returns:
-            festim.SurfaceSubdomain1D: the left surface subdomain
+            festim.SurfaceSubdomain1D: the left surface subdomain. Its id is
+            automatically allocated when the model is initialised
        """
-        return SurfaceSubdomain1D(id=-1, x=self.borders[0])
+        return SurfaceSubdomain1D(id=None, x=self.borders[0])

-    @property
+    @cached_property
    def right(self):
        """The right surface of the 1D volume subdomain.

        Returns:
-            festim.SurfaceSubdomain1D: the right surface subdomain
+            festim.SurfaceSubdomain1D: the right surface subdomain. Its id is
+            automatically allocated when the model is initialised
        """
-        return SurfaceSubdomain1D(id=-2, x=self.borders[1])
+        return SurfaceSubdomain1D(id=None, x=self.borders[1])


@user-name-owen @ee-nn how does it work if right is cached but in the meantime self.borders change? say

vol = F.VolumeSubdomain1D(..., borders=[1, 2]) print(vol.right.x) # output: 2 vol.borders = [2, 3] print(vol.right.x) # output: ????

maybe this is an edge case we don't have to consider at all. I've never seen anyone change the borders of a domain programmatically like this

user-name-owen added 6 commits October 21, 2026 13:59

Add left/right surface properties

95074d3

Add files via upload

c10219e

Delete test_subdomains.py

8ba2852

Remove misplaced file

Delete volume_subdomain.py

0dd046c

Remove misplaced file

Add left/right surface properties

b98976d

Add test for left/right properties

e966933

ee-nn requested changes Oct 25, 2026

View reviewed changes

user-name-owen added 5 commits November 8, 2026 19:43

Fix ids and cache surfaces

2fc31e0

Fix surface id allocation

29b2027

Update surface id tests

e2da1e5

Document left/right surfaces

135aff3

Merge branch 'main' into left-right-subdomains

3202f80

RemDelaporteMathurin reviewed Nov 9, 2026

View reviewed changes

Conversation

user-name-owen commented Oct 21, 2026

Description

Summary

Related Issues

Motivation and Context

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

ee-nn commented Oct 23, 2026

Uh oh!

ee-nn 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!

user-name-owen commented Nov 8, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov Bot commented Oct 21, 2026 •

edited

Loading