Publish detector view images as float32 - #1292
Merged
Merged
Conversation
da00 is uncompressed, so the image dtype is the wire size: NMX's three 640x640 panels cost 19.7 MB per update as float64, and a 512x512 Timepix3 view 4.2 MB. Every event carries a weight of exactly 1.0 in eight bytes. The cast has to be at the publishing boundary rather than at the source. scipp preserves dtype through bins.sum(), so float32 weights would make the accumulated histogram float32 too -- and float32 stops incrementing at 2**24, which a long-running cumulative image can reach, silently. Casting the result instead rounds a displayed value by at most one part in 2**24, with nothing accumulating on top. Scoped to the count images rather than Da00Serializer: other da00 payloads carry physical quantities and timestamps where float32 is not automatically appropriate. Fixes #1290
Member
Author
|
LGTM |
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 #1290.
da00is uncompressed, so the image dtype is the wire size. Detector views published float64 becauseToNXevent_dataallocates its per-event weight buffer as float64 andbins.sum()carries that dtype through to the image — every event contributing a weight of exactly 1.0 in eight bytes. Halving the payload also halves what the dashboard's buffers hold per frame (#1274).Measured through the real
da00path: a 640x640 panel drops from 3.29 MB to 1.65 MB per update, so NMX's three panels go from 9.9 MB to 4.9 MB. A float32 array roundtrips throughscipp_to_da00/da00_to_scippidentically.The cast is at the publishing boundary rather than at the source. Making the weights float32 would make the accumulated histogram float32, and float32 stops incrementing at 2**24 — a long-running cumulative image can reach that and silently stop counting. Casting the result instead rounds a displayed value by at most one part in 16 million, with nothing accumulating on top. So the histogram, both accumulators, the count scalars,
spectrum_viewandroi_spectraall stay float64; only the two count images are cast.Scoped to the count images rather than
Da00Serializer, since otherda00payloads carry physical quantities and timestamps where float32 is not automatically appropriate.The float64 per-event weights buffer is left alone: #1290 scopes it out as a separate saving, and flipping it is precisely what would make the accumulation float32 — it would need the accumulator to upcast deliberately.
Test plan
New unit tests cover the image being float32 with and without pixel weighting (the weighted path matters: weights are already float32, so
image / weightspromotes back to float64 before the cast), and the accumulated histogram staying float64 and still counting exactly past 2**24. The factory-level integration test additionally pins the count scalars as not cast.Four existing tests compared an image against float64 via
sc.isclose/sc.full, which raisesDTypeErroron mixed dtypes rather than failing a comparison; they now compare values or the expected dtype.Unrelated to this change, five bifrost qmap tests fail identically on the unmodified branch point in a freshly resolved venv —
ess.spectroscopy'smonitor_wavelength_datais missing akeep_event_time_offsetargument.Worth a look in a running dashboard, since this changes what every detector view plot receives: