fix: route multipage frames by the acquisition XML, and carry z forward across frames - #134
Conversation
Two defects in reading PrairieView acquisitions. Multipage routing derived page offsets from a global channel-by-plane stride and applied them to a file list that get_prairieview_filenames had already filtered by channel and plane, so the offset was applied twice. With two channels roughly half the output frames were never assigned, and because the output array is allocated with np.empty those frames held uninitialised memory rather than raising. get_prairieview_file_pages now returns the filename and page the XML names for each frame, and the multipage branch asserts one write per output frame. Z positions are read from a PVStateShard, which records state *changes*: a frame whose z has not moved omits positionCurrent entirely, so counting the elements counts re-declarations rather than planes. A 3-plane bidirectional-Z recording therefore failed outright with 'Number of z fields does not match number of depths'. Z is now carried forward from the document-level shard.
Two gaps in the frame-routing change. A depth declared neither at the document level nor in any preceding frame left None in the position list. The length assert compares counts, so it could not see the None, and the value reached fieldZ and then ScanInfo.Field.field_z. It now raises where the value is produced. num_frames is floored by num_planes, so an acquisition that stopped part-way through its last cycle names more frames for the early planes than the late ones. Refusing outright made such a recording unprocessable; every plane must come out the same length, so the incomplete cycle is dropped with a warning. Fewer frames than expected still raises, since that means data is missing rather than a cycle being partial. Version 0.8.4 with a changelog entry, which the previous commit omitted.
ttngu207
left a comment
ttngu207
left a comment
There was a problem hiding this comment.
Approving. This is the right fix done the right way — reading the (filename, page) pairs the XML names, instead of recomputing them from a stride, removes the class of bug rather than the instance. The written coverage assertion is the part I like most: a movie of nominally correct length full of uninitialised memory is exactly the failure that should be loud.
I checked the back-compat claim rather than taking it. For single-channel/single-plane the old formula gives page_indices = [0, 1, 2, ...], identical to the new mapping — so there's no regression risk on any session we currently have. That's what makes this safe to land ahead of the rest of the chain.
Two things.
1. Bound the truncation — one line, worth doing here. The new len(file_pages) > num_frames branch warns and truncates. A partial final cycle can only ever leave an excess of one frame. A 2x excess means the channel/plane filter didn't filter, and then this silently writes the first half of a wrong movie — the same defect class the PR is fixing.
if len(file_pages) > self.meta["num_frames"] + 1:
raise ValueError(...) # not a partial cycle - the filtering is wrong2. Follow-up, not blocking: two traversals that have to agree. get_prairieview_file_pages duplicates get_prairieview_filenames' traversal verbatim — the bidi branch and the multiplane branch both copied. Two pieces of code that must agree about which frames belong to a (plane, channel) is the fragility that produced this bug in the first place.
They already disagree, in fact: get_prairieview_filenames walks .//Sequence/Frame, this walks .//Sequence[@cycle]. A Sequence with no cycle attribute makes them diverge, and the new < num_frames check would then raise on valid data. One traversal fixes it — filenames := unique(f for f, _ in file_pages(...)).
One note for the record: this raises where it previously passed, so it's a behavior change for every element consumer, not just us. Correct behavior, but 0.8.3 -> 0.8.4 undersells it.
Addresses review on datajoint#134. get_prairieview_file_pages filtered on Sequence[@cycle] while get_prairieview_filenames and num_frames count every Sequence. A Sequence carrying no cycle attribute therefore made the two disagree, and the frame-count check added in the previous commit would reject valid data. Both now walk the same elements; a Sequence without a cycle takes the forward plane ordering, which is the only ordering it can have. A partial final cycle can leave at most one extra frame, since num_frames is floored by num_planes. A larger excess means the plane or channel filter did not filter, where truncating would write the leading fraction of the wrong movie, so that now raises. Minor version rather than patch: inputs that previously passed silently now raise, which is a behavior change for every consumer of the loader.
2 participants
What this covers
Two defects in reading PrairieView acquisitions, both found while validating two-photon
sessions against the lab's own analysis.
Multipage frame routing
get_prairieview_filenamesalready filters the file list by channel and plane. The multipagebranch then derived page offsets from a global channel-by-plane stride and applied them to
that already-filtered list, so the offset was applied twice.
With two channels this left roughly half the output frames unassigned. Because the output array
is allocated with
np.empty, those frames carried uninitialised memory instead of raising — amovie of nominally correct length whose content was arbitrary.
get_prairieview_file_pages()now returns the(filename, page)pair the acquisition XML namesfor each frame, and the multipage branch writes exactly those, asserting one write per output
frame so a mismatch between XML and disk fails instead of passing silently.
Z positions across frames
Z is read from a
PVStateShard, which records state changes. A frame whose z has not movedomits
positionCurrententirely, so counting those elements counts re-declarations rather thanplanes. A 3-plane bidirectional-Z recording therefore failed outright with "Number of z fields
does not match number of depths". Z is now carried forward from the document-level shard.
Verification
Regression tests live in the workflow repository, since element-interface has no test suite:
8 cases on a synthetic two-channel multipage acquisition, built with
tifffileonly — no CaImAnand no database. All 8 pass against this change. Against the previous loader 5 fail, including
both tests that check frame content.
The coverage test truncates the source file so the XML names a page that does not exist on disk,
which is the case the previous implementation passed through silently.
Not covered
The affected acquisitions are two-channel. Every session currently available to us is
single-channel, so the fix is verified against a constructed acquisition rather than recorded
data. A real two-channel session has been requested.