Repository navigation
Keep the physical dimension of mappings in Domain.export / Domain.from_file - #201
Merged
Merged
Conversation
# Conflicts: # CHANGELOG.md # sympde/topology/tests/test_topology.py
yguclu
self-requested a review
October 22, 2026 13:47
This was referenced Oct 22, 2026
yguclu
requested changes
Oct 22, 2026
yguclu
left a comment
yguclu
left a comment
Member
There was a problem hiding this comment.
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
Member
|
@syahra712 Have you read my comments from last week? If not, you should check your notification settings |
- Domain.todict() raises ValueError when the mappings disagree on pdim - Add test_export_mappings_with_different_pdim - Shorten the Fixed entry and add a Changed entry for the ldim/pdim format change - Add myself to AUTHORS
Contributor
Author
|
@yguclu Sorry, I missed your review last week. That one's on me, and I've fixed how I track notifications. All of your suggestions are in now:
All 21 topology tests pass locally. |
yguclu
reviewed
Oct 29, 2026
This was referenced Oct 29, 2026
Member
yguclu
added a commit
that referenced
this pull request
Oct 30, 2026
## Summary Write the HDF5 files of the unit tests to the `tmp_path` fixture instead of the working directory, so that the tests can run in parallel without the `xdist_group` marker (fixes #204). ## Problem The six export/import tests of `sympde/topology/tests/test_gallery.py` all wrote and read the same file `domain.h5` in the working directory. They relied on `@pytest.mark.xdist_group('h5py')` and `--dist loadgroup` to run one after another on the same worker. pytest 9.2.0 (released 2026-10-27) changed how node IDs are stored, and pytest-xdist 3.8.0 can no longer assign tests to their group (see pytest-dev/pytest-xdist#1386, fixed in pytest-dev/pytest-xdist#1388 but not released yet). The grouped tests were therefore spread over the workers and failed randomly with `BlockingIOError` or `OSError` when two of them opened `domain.h5` at the same time. This is what currently makes CI fail in #201. ## Changes - Each test in `test_gallery.py` that exports a domain writes it to its own `tmp_path`, and the `xdist_group('h5py')` markers are removed. - `test_topology_1` in `test_topology.py` writes `omega.h5` to `tmp_path` too. - The removal of the output files in `teardown_module` is no longer needed and is deleted. - Remove `--dist loadgroup` from the pytest commands in CI and in the README, since no test uses the `xdist_group` marker anymore. - Add a `[DEVELOPER]` entry to the CHANGELOG. ## Validation With pytest 9.2.0 and pytest-xdist 3.8.0, `pytest -n auto --dist loadgroup --pyargs sympde -ra` passed 5 times in a row (180 passed, 2 skipped) and left no files in the working directory. On `master`, the same command failed in all 3 runs, with 2 or 3 failing tests each time. CI passes in all jobs with the final command `pytest -n auto --pyargs sympde -ra`.
yguclu
approved these changes
Oct 30, 2026
yguclu
added a commit
that referenced
this pull request
Oct 30, 2026
#201 renamed the local variable dim to ldim in Domain.from_file, but the new code path for topological domains still used dim, which raised a NameError when reading a domain without dtype.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #200, following the proposal in the issue:
Domain.todict(and soDomain.export) writesldimandpdiminstead ofdim.pdimcomes from the domain's mappings, or equalsldimfor an unmapped domain. AValueErroris raised if the mappings have different physical dimensions.Domain.from_filecreates each mapping withldimandpdim.dimkey raises aValueErrorsaying it was written by an older version of SymPDE and thatdimmust be replaced byldimandpdim. This is a breaking change.# TODO add pdimcomment is removed.AUTHORS.Tests in
test_topology.py:test_export_surface_mapping: a round trip ofMapping('F', ldim=2, pdim=3)(Square('Omega'))keepspdim == 3.test_from_file_with_old_dim_key: a file withdimraises the error.test_export_mappings_with_different_pdim: exporting a domain whose mappings have different physical dimensions raises aValueError.The full test suite passes (183 passed, 2 skipped).