Only write ImageSeries.num_samples when it was explicitly provided - #2239
Open
adityasingh2400 wants to merge 3 commits into
Open
Only write ImageSeries.num_samples when it was explicitly provided#2239adityasingh2400 wants to merge 3 commits into
adityasingh2400 wants to merge 3 commits into
Conversation
ImageSeries.num_samples falls back to the inherited TimeSeries.num_samples property, len(data), when the user did not supply a value. The default ObjectMapper persisted that derived value, so every ImageSeries with internal data wrote a num_samples dataset the user never set, and HDMF widened the Python int to uint64 against the schema's uint32 dataset and emitted a DtypeConversionWarning. Map the write path to the private _num_samples so the dataset is written only when num_samples was explicitly provided, and cast an in-range explicit value to uint32 so it is written with the schema dtype. The read path is unchanged, so files that do contain num_samples still map it back to the constructor argument. Fix NeurodataWithoutBorders#2227
4 tasks
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.
Motivation
Fix #2227.
ImageSeries.num_samplesis a property that falls back to the inheritedTimeSeries.num_samples, i.e.len(data), when the user did not supply a value. There is no custom mapping for that dataset, so the defaultObjectMapperread the property and persisted the derived value. EveryImageSeries(andOpticalSeries,OnePhotonSeries,TwoPhotonSeries) built from internal data therefore wrote anum_samplesdataset the user never set, and because the derived value is a Pythonintand the schema dataset isuint32, HDMF widened it touint64and emittedDtypeConversionWarning: Spec 'ImageSeries/num_samples': Value with data type int64 is being converted to data type uint64 (min specification: uint32). That warning fires across the image and ophys integration tests today.The schema documents
num_samplesas the frame count that cannot otherwise be recovered, which is theformat='external'plusstarting_time/ratecase, so writing a value derived fromlen(data)is redundant. This matches the conclusion in #2215 that persisting it for internal data was an oversight.Fix
ImageSeriesMapnow maps the write path of thenum_samplesspec to the private_num_samples, which holds a value only whennum_sampleswas explicitly passed to the constructor. When it was not set, nothing is written. The read path is untouched, so a file that does containnum_samplesstill maps it back to thenum_samplesconstructor argument and round-trips unchanged. The in-memoryImageSeries.num_samplesproperty is also unchanged and still returns the frame count derived from the data.An
object_attroverride additionally casts an explicitly provided, in-range value touint32so it is written with the schema dtype rather than being widened touint64. Out-of-range values are passed through untouched so HDMF still reports the mismatch.How to test the behavior?
Four new tests in
tests/integration/hdf5/test_image.pycover this. They fail ondevand pass with the change:After the change,
pytest tests/integration/hdf5/test_image.py tests/integration/hdf5/test_ophys.pypasses with zeronum_samplesdtype-conversion warnings, down from the previous count.I deliberately left out the consistency check you suggested in #2215, where an explicitly provided
num_sampleson an internalImageSeriesshould be validated against the number of frames in the data. Happy to add it here or in a follow-up if you would like it.Checklist
ruff check . && codespellfrom the source directory.