[syahra712]

Fixes #200, following the proposal in the issue:

• Domain.todict (and so Domain.export) writes ldim and pdim instead of dim. pdim comes from the domain's mappings, or equals ldim for an unmapped domain.
• Domain.from_file creates each mapping with ldim and pdim.
• A file with the old dim key raises a ValueError saying it was written by an older version of SymPDE and that dim must be replaced by ldim and pdim.
• The # TODO add pdim comment is removed and a CHANGELOG entry is added under Fixed.

Tests in test_topology.py:

• test_export_surface_mapping: a round trip of Mapping('F', ldim=2, pdim=3)(Square('Omega')) keeps pdim == 3.
• test_from_file_with_old_dim_key: a file with dim raises the error.

sympde/topology tests pass locally (72 passed).

This changes the same from_file line as #196 (Mapping → SymbolicMapping), so whichever PR lands second needs a one-line rebase.

[yguclu]

Thanks for this contribution!

I have posted a couple of suggestions below.

If you are comfortable with adding your full name to the bottom of the AUTHORS file, please feel free to do so. We do not list contributors under their GitHub username or other aliases

[yguclu]

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

[yguclu]

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.

[yguclu]

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()`.

[yguclu]

Here we should add a point:

Suggested change

	- #200: Store `ldim` and `pdim` instead of `dim` in the topology of domain files. This is a breaking change: `Domain.from_file()` rejects files with `dim`.