No way to read a derived property with a default (#178) - #181
Merged
Conversation
A recipe whose conditions do not match, or whose parameters the files do not contain, leaves its property unset, so an absent property and a misspelled one are the same AttributeError and neither can be given a default. Which properties exist is ParaVision-version dependent, so this is the normal case, not an edge: over the 1626 reconstructions of our corpus that load, TE does not resolve for 63, TR for 54, extent/resolution for 35, imaging_frequency for 38. The only workaround is try/except at every access, which also swallows typos and genuine errors raised by a recipe's cmd -- so a consumer ends up reading the underlying parameters directly instead, which is the point of having derived properties at all. `Dataset.get(name, default=None)` returns the default where the recipe does not resolve, matching the shape `JCAMPDX.get_value` already establishes for parameters. A name no configuration declares still raises, so a misspelling does not quietly become the default, and a property that fails for a reason of its own -- `affine` on a scan with no image geometry -- still raises that reason rather than reporting an absence. Telling the two apart needs the set of declared property names, which `_add_property` now records whether or not the recipe resolves. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NuK1cZi8U54WXAdXMmGpzy
…default # Conflicts: # test/test_api.py
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 #178.
The issue's optional distinction is implemented, because it is what makes
getsafe to reach for: a name no configuration declares still raises, and a property that fails for a reason of its own still raises that reason rather than reporting an absence:Telling "declared but unresolved" from "never a property" needs the set of declared names, so
_add_propertyrecords each name whether or not its recipe resolves (_declared_properties, cleared byunload_propertiesalongside_properties).Measured on our corpus
1626 vendor 2dseq reconstructions (PV 5.1 – 360.3.7):
get()== attribute where it resolvesTETRextent,resolutionimaging_frequencygetchanges nothing where the property resolves. ForTE,TRandimaging_frequencythe underlying parameter is absent in every raising case, confirming the information content is the same and only the failure mode differs.Note for review
Dataset.__getitem__returns a parameter whilegetreturns a property, sod["TE"]raisesKeyErrorwhered.get("TE")returns a value. That asymmetry is inherent to naming itget— the docstring says which layer it reads, andget_propertywould be unambiguous if you prefer it. I kept the name the issue asked for.Suite: 2112 passed, 12 skipped. The 5
test_affine_agrees_with_the_paravision_nifti_sformfailures are the same ones master has.🤖 Generated with Claude Code
https://claude.ai/code/session_01NuK1cZi8U54WXAdXMmGpzy