Skip to content

Keep the physical dimension of mappings in Domain.export / Domain.from_file #201

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
syahra712 wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Keep the physical dimension of mappings in Domain.export / Domain.from_file #201

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
aed5382
Test that Domain.export keeps the physical dimension of the mappings
syahra712 Oct 16, 2026
1cf6298
Store ldim and pdim in the exported domain metadata
syahra712 Oct 16, 2026
79811e7
Merge remote-tracking branch 'origin/master' into fix/domain-export-pdim
syahra712 Oct 20, 2026
aeed096
Merge branch 'master' into fix/domain-export-pdim
yguclu Oct 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ All notable changes to this project will be documented in this file.

### Fixed

- #200: Keep the physical dimension of the mappings in `Domain.export` and `Domain.from_file`. The `topology.yml` metadata now stores `ldim` and `pdim` instead of `dim`, and `Domain.from_file` raises an error for files with the old `dim` key.

@yguclu yguclu Oct 22, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would split this into two separate points. A new one should go under "Changed" (see my other comment to this file) and this one should become much shorter:

Suggested change
- #200: Keep the physical dimension of the mappings in `Domain.export` and `Domain.from_file`. The `topology.yml` metadata now stores `ldim` and `pdim` instead of `dim`, and `Domain.from_file` raises an error for files with the old `dim` key.
- #200: Keep the physical dimension of the mappings in `Domain.export()` and `Domain.from_file()`.

- #140: Fix the rendering of the `Domain.join()` API documentation.
- #166: Restore warning-free documentation builds, including bibliography and mathematical notation support.
- #106: Preserve symbol assumptions and free-symbol information with modern SymPy.
Expand Down
17 changes: 12 additions & 5 deletions sympde/topology/domain.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@
# TODO fix circular dependency between domain and mapping
if TYPE_CHECKING:
from sympde.topology.mapping import Mapping
# TODO add pdim

iterable_types = (tuple, list, Tuple, Union)

Expand Down Expand Up @@ -336,7 +335,9 @@ def get_interior(self, name):

def todict(self):
name = str(self.name)
dim = str(self.dim)
ldim = str(self.dim)
mappings = [m for m in self.mappings.values() if m is not None]
pdim = str(mappings[0].pdim) if mappings else ldim

@yguclu yguclu Oct 22, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If todict takes pdim from the first mapping, a domain whose patches have different physical dimensions is exported with a wrong pdim for some patches (this is currently possible because Domain.join does not check pdim, see #140, which will be fixed in #188). I recommend that we raise an error in that case:

Suggested change
pdim = str(mappings[0].pdim) if mappings else ldim
pdim = {m.pdim for m in self.mappings.values() if m is not None}
if len(pdims) > 1:
raise ValueError(f'all mappings must have the same physical dimension, got {sorted(pdims)}')
pdim = str(pdims.pop()) if pdims else ldim

interior = self.interior.todict()
boundary = self.boundary.todict()
connectivity = self.connectivity.todict()
Expand All @@ -346,7 +347,8 @@ def todict(self):
dtype = 'None'

d = {'name': name,
'dim': dim,
'ldim': ldim,
'pdim': pdim,
'dtype': dtype,
'interior': interior,
'boundary': boundary,
Expand Down Expand Up @@ -404,8 +406,13 @@ def from_file(cls, filename):
h5 = h5py.File( filename, mode='r' )
yml = yaml.load( h5['topology.yml'][()], Loader=yaml.SafeLoader )

if 'dim' in yml:
raise ValueError(f"'{filename}' was written by an older version of SymPDE:"
" replace its 'dim' key with the keys 'ldim' and 'pdim'")

domain_name = yml['name']
dim = int(yml['dim'])
ldim = int(yml['ldim'])
pdim = int(yml['pdim'])
dtype = yml['dtype']
d_interior = yml['interior']
d_boundary = yml['boundary']
Expand All @@ -422,7 +429,7 @@ def from_file(cls, filename):

constructors = [globals()[dt['type']] for dt in dtype]
interiors = [cs(i['name'], **dt['parameters']) for cs,i,dt in zip(constructors, d_interior, dtype)]
mappings = [Mapping(I['mapping'], dim=dim) if I.get('mapping', "None") != "None" else None for I in d_interior]
mappings = [Mapping(I['mapping'], ldim=ldim, pdim=pdim) if I.get('mapping', "None") != "None" else None for I in d_interior]
domains = [mapping(i) if mapping else i for i,mapping in zip(interiors, mappings)]
patch_index = {I.name:ind for ind,I in enumerate(interiors)}

Expand Down
29 changes: 29 additions & 0 deletions sympde/topology/tests/test_topology.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
import os

import h5py
import numpy as np
import pytest
import yaml
from sympy import preorder_traversal
from sympy.core import Basic

Expand Down Expand Up @@ -113,6 +117,31 @@ def test_topology_1():

assert D.todict() == Omega.todict()

#==============================================================================
def test_export_surface_mapping(tmp_path):
domain = Mapping('F', ldim=2, pdim=3)(Square('Omega'))

filename = str(tmp_path / 'surface.h5')
domain.export(filename)
D = Domain.from_file(filename)

assert D.mapping.ldim == 2
assert D.mapping.pdim == 3
assert D.todict() == domain.todict()

#==============================================================================
def test_from_file_with_old_dim_key(tmp_path):
yml = Mapping('F', dim=2)(Square('Omega')).todict()
yml['dim'] = yml.pop('ldim')
del yml['pdim']

filename = str(tmp_path / 'old.h5')
with h5py.File(filename, mode='w') as h5:
h5['topology.yml'] = np.array(yaml.dump(data=yml, sort_keys=None), dtype='S')

with pytest.raises(ValueError, match="older version of SymPDE"):
Domain.from_file(filename)

@yguclu yguclu Oct 22, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see my comment to Domain.todict(). Here we should add a test which joins a patch mapped with pdim=3 and a patch mapped with pdim=2, and checks that exporting the result raises a ValueError:

#==============================================================================
def test_export_mappings_with_different_pdim(tmp_path):
    A = Mapping('FA', ldim=2, pdim=3)(Square('A'))
    B = Mapping('FB', ldim=2, pdim=2)(Square('B'))

    with pytest.raises(ValueError):
        domain = Domain.join([A, B], [((0, 0, 1), (1, 0, -1), 1)], name='Omega')
        domain.export(str(tmp_path / 'omega.h5'))

Both Domain.join and export are inside the pytest.raises block on purpose: today the error is raised by export, and once #140 is fixed it will be raised earlier by Domain.join, so the test keeps passing in both cases.

#==============================================================================
def test_domain_1():
Omega_1 = InteriorDomain('Omega_1', dim=2)
Expand Down
Loading