You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
AI-generated issue, opened from #418 and not verified against real MACSima data. The
code pointers are accurate; the question at the end needs someone who knows what the
MACSima software writes into these fields.
The values end up on ChannelMetadata.translation_x / translation_y and are consumed by MultiChannelImage._pad_images(), which normalises them against their minimum and uses the
result directly as da.pad widths, i.e. as a number of pixels:
1. int() truncates towards zero instead of rounding.int(0.9) == 0 and int(-0.9) == 0, so a plane at x = 10.7 is placed at 10 and the direction of the error
depends on the sign of the position. After the normalisation against the minimum the residual
is below one pixel per channel, so this alone is minor — round() would halve the worst case
and make it sign-symmetric.
2. The positions are physical coordinates, but they are used as pixel counts. In OME, Plane.position_x comes with a Plane.position_x_unit, and Pixels.physical_size_x gives the
size of a pixel in the same kind of unit (17.0 µm in the OMAP test data). Nothing in the
reader converts between the two, so if a MACSima file writes stage positions in micrometres the
padding is off by the pixel size — a factor of 17 for these datasets — rather than by a fraction
of a pixel. If instead the software writes pixel offsets there (position_x_unit is REFERENCEFRAME, the OME default, i.e. unspecified, in the files I looked at), the current code
is right and only point 1 applies.
Both small test datasets (OMAP10_small, OMAP23_small) have position_x = None on every
plane, so _get_translations() returns (0, 0) there and this path is exercised by unit tests
only, never by the data-based ones.
What a fix would entail
Depending on the answer to "what unit does MACSima write":
if the positions are already in pixels: replace int(...) with round(...) in _get_translations() and extend test_get_translations_returns_correct_values with a
fractional case (e.g. position_x=10.7, position_y=0.9 → {"translation_x": 11, "translation_y": 1});
if they are physical lengths: read position_x_unit / position_y_unit and Pixels.physical_size_x / physical_size_y alongside the positions, convert to pixels
(ome_types exposes the unit enums; parse_physical_size() in readers/_utils/_utils.py already does this kind of lookup for the physical size), and round
at the end. _get_translations() currently only receives the OME object, which is enough for
both. ChannelMetadata.translation_x stays an int, so _pad_images() is unaffected.
either way, it would be good to have a data-based test: none of the datasets currently in CI
has plane positions, so this would need either a file with real positions or a synthetic TIFF
written with tifffile.imwrite(..., metadata={...}) in the style of test_images_with_invalid_ome_metadata_are_excluded.
Context
#418 originally rounded the positions as part of fixing the regressions from #411, but that
changes reader output on a point nobody has confirmed, so it was pulled out of the PR and filed
here instead. The current behaviour on main is unchanged: truncation.
Note
AI-generated issue, opened from #418 and not verified against real MACSima data. The
code pointers are accurate; the question at the end needs someone who knows what the
MACSima software writes into these fields.
What the code does
_get_translations()reads the position of the first plane of the OME metadata and turns itinto an integer —
src/spatialdata_io/readers/macsima.py:The values end up on
ChannelMetadata.translation_x/translation_yand are consumed byMultiChannelImage._pad_images(), which normalises them against their minimum and uses theresult directly as
da.padwidths, i.e. as a number of pixels:Two things look off
1.
int()truncates towards zero instead of rounding.int(0.9) == 0andint(-0.9) == 0, so a plane atx = 10.7is placed at 10 and the direction of the errordepends on the sign of the position. After the normalisation against the minimum the residual
is below one pixel per channel, so this alone is minor —
round()would halve the worst caseand make it sign-symmetric.
2. The positions are physical coordinates, but they are used as pixel counts. In OME,
Plane.position_xcomes with aPlane.position_x_unit, andPixels.physical_size_xgives thesize of a pixel in the same kind of unit (
17.0 µmin the OMAP test data). Nothing in thereader converts between the two, so if a MACSima file writes stage positions in micrometres the
padding is off by the pixel size — a factor of 17 for these datasets — rather than by a fraction
of a pixel. If instead the software writes pixel offsets there (
position_x_unitisREFERENCEFRAME, the OME default, i.e. unspecified, in the files I looked at), the current codeis right and only point 1 applies.
Both small test datasets (
OMAP10_small,OMAP23_small) haveposition_x = Noneon everyplane, so
_get_translations()returns(0, 0)there and this path is exercised by unit testsonly, never by the data-based ones.
What a fix would entail
Depending on the answer to "what unit does MACSima write":
int(...)withround(...)in_get_translations()and extendtest_get_translations_returns_correct_valueswith afractional case (e.g.
position_x=10.7, position_y=0.9→{"translation_x": 11, "translation_y": 1});position_x_unit/position_y_unitandPixels.physical_size_x/physical_size_yalongside the positions, convert to pixels(
ome_typesexposes the unit enums;parse_physical_size()inreaders/_utils/_utils.pyalready does this kind of lookup for the physical size), and roundat the end.
_get_translations()currently only receives theOMEobject, which is enough forboth.
ChannelMetadata.translation_xstays anint, so_pad_images()is unaffected.has plane positions, so this would need either a file with real positions or a synthetic TIFF
written with
tifffile.imwrite(..., metadata={...})in the style oftest_images_with_invalid_ome_metadata_are_excluded.Context
#418 originally rounded the positions as part of fixing the regressions from #411, but that
changes reader output on a point nobody has confirmed, so it was pulled out of the PR and filed
here instead. The current behaviour on
mainis unchanged: truncation.