🐛 resolve get_physical_dim across multiple bases when unambiguous - #483
Open
xroynard wants to merge 4 commits into
Open
🐛 resolve get_physical_dim across multiple bases when unambiguous#483xroynard wants to merge 4 commits into
xroynard wants to merge 4 commits into
Conversation
…biguous get_physical_dim previously delegated base resolution to resolve_base, which raises when several bases exist and no default is set. But the physical dimension is a per-base CGNS attribute that bases very often share, so the ambiguity is not always real. When no base is given and every base agrees on the physical dimension, return that common value; only raise (with an explicit message) when the dimensions actually differ. Passing an explicit base keeps the previous behaviour.
xroynard
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
tdaniel06
approved these changes
Jul 31, 2026
casenave
requested changes
Jul 31, 2026
Comment on lines
+757
to
+759
| try: | ||
| base = self.resolve_base(base, time) | ||
| except KeyError: |
Member
There was a problem hiding this comment.
Is it possible to have an explicit branching instead of relying on try/except ?
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
3 participants
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.
Context
Fixes #482.
Sample.get_physical_dim()currently delegates base resolution toresolve_base(), which raises as soon as a Sample holds more than one base and no default base is set. This forces callers to set a default base even when the answer is unambiguous.The physical dimension is stored per base in the CGNS datamodel (the 2nd integer of the
CGNSBase_tnode value, set bynewCGNSBaseininit_base), and nothing constrains different bases to agree. However, in the overwhelming majority of real Samples all bases do share the same physical dimension (e.g. a volume base and its surface/line boundary bases all live in the same ambient space). So requiring a default base in that case is unnecessary friction.Change
get_physical_dim(base=None)now:Globalbase);ValueErrorasking the caller to specify a base only when the physical dimensions actually differ across bases.Passing an explicit
basekeeps the previous behaviour unchanged.get_topological_dim()is intentionally left as-is: topological dimensions legitimately differ between bases (a 3D volume base vs. a 2D surface base), so the ambiguity there is real.This is backward compatible: previously-working calls keep working, and calls that used to raise on multi-base Samples now succeed when the value is unambiguous.
Tests
Added
test_get_physical_dim_multibase(agreeing bases → common value returned, topological dim still raises) andtest_get_physical_dim_multibase_diverging(differing bases → explicitValueError, explicit base resolves it).