NGFF v06 transformations - #1182
Tomaz-Vieira wants to merge 26 commits into
Conversation
0dfccb5 to
f57dc1d
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## transformation_manager #1182 +/- ##
=========================================================
Coverage ? 92.22%
=========================================================
Files ? 55
Lines ? 8654
Branches ? 0
=========================================================
Hits ? 7981
Misses ? 673
Partials ? 0
🚀 New features to boost your workflow:
|
| multiscale = oz.OMEZarrMultiscale.from_ome_zarr(str(store)) | ||
| assert isinstance(multiscale, oz.OMEZarrMultiscale) # disambiguate from OMEZarrLabel | ||
|
|
||
| name_to_cs: dict[str, CoordSystem] = {} |
There was a problem hiding this comment.
Reminder: later when we work with Scenes, the coordinate system name is not enough for uniquely identifying a CS. It will be the combniation of path where the cs is defined, and the name.
There was a problem hiding this comment.
I saw later in that in vert.py there is already some logic needed for this (the CoordinateSystemIndentifier usage.
| continue | ||
| coords = coords.merge( | ||
| xr.Coordinates.from_xindex( | ||
| RangeIndex.linspace( |
There was a problem hiding this comment.
RangeIndex is available from xarray 2025.04.0 (pydata/xarray@3816901), so we need to bump our min dependencies in pyproject.toml
| assert transf.input.path is not None | ||
| in_cs = CoordSystem(name=str(transf.input.name), axes=[Axis(name=ax.name, type=ax.type) for ax in out_cs.axes]) | ||
|
|
||
| ozm_seq = ozm06trans.Sequence(transformations=ds_md.coordinateTransformations) |
There was a problem hiding this comment.
IIRC, according to the specs, here we either have a Sequence (of Scale + Translation) or a Scale. We need to double check.
There was a problem hiding this comment.
Anyway the logic of doing .transform_points() is correct in both cases
| class AxisParsingException(Exception): | ||
| pass |
There was a problem hiding this comment.
I like it here. Just for awareness, in the transformation manager codebase the exceptions are in a separate exceptions.py file. https://github.com/scverse/spatialdata/pull/1164/changes
We can choose to keep things local unless there are many exceptions, in that case it is better to place in a dedicated files.
| if not isinstance(model.unit, str): | ||
| raise AxisParsingException("Can't handle axis unit") |
There was a problem hiding this comment.
Something to keep in mind for when we'll try to read legacy data. In the old CS we were settings unit="unit". This is not allowed by the specs (having "unit" is a SHOULD, but if they are specified they need to be real units).
There was a problem hiding this comment.
But also here, shouldn't we allow for model.unit to be None?
There was a problem hiding this comment.
shouldn't we allow for model.unit to be None?
good catch!
Regarding saving unit="unit" in the legacy model, I suppose we could read it in as None
| class LegacyAxes: | ||
| pass |
There was a problem hiding this comment.
unused for now; placeholder for later?
| if len(self.axes) != len({axis.name for axis in self.axes}): | ||
| raise ValueError("Axes names must be unique") |
There was a problem hiding this comment.
Can we delegate all the validation to ozm, including this one?
There was a problem hiding this comment.
Another issue with delegating to ozm is that it can only validate based on their own models, so we'd have to convert e.g. our Axis type into their ozm.Axis to do the validation, which is also kinda clunky 🤔
There was a problem hiding this comment.
Maybe a custom Exception would help here? Also would be good to add some contextual information like the name of the coordinate system and name of the axes to the exception. This would help users to understand/communicate about this error when a user loads an NGFF 0.6 ome-zarr file and gets this exception.
I guess such contextual information would help for all our custom exceptions.
There was a problem hiding this comment.
yup, good idea. I was abusing ValueError quite a bit
| ) | ||
|
|
||
| @classmethod | ||
| def try_from_model_or_default[T](cls, model: ozi.CoordinateSystem | None, *, default: T) -> CoordSystem | T: |
There was a problem hiding this comment.
Who is going to use this/how?
There was a problem hiding this comment.
The logic for ensuring that default has unique names is something that would live outside this class. I wonder if then even the whole 3-line implementation of this function should live outside.
There was a problem hiding this comment.
yeah, this is a wonky attempt to simplify code elsewhere. I'll see if I can clean it up
| from collections.abc import Sequence | ||
| from typing import Final, Literal | ||
|
|
||
| import ome_zarr.classes.image as ozi |
There was a problem hiding this comment.
How about being more explicit and import directly from ome-zarr-models-py? WDYT?
There was a problem hiding this comment.
Also, this will currently import Image and Axis that implement NGFF v0.5. Please mention the development branches of dependencies against which this PR is being developed in the PR description.
There was a problem hiding this comment.
It's all supposed to be on top of 0.6, but I'll add it to the PR description as well
|
|
||
| @abstractmethod | ||
| def to_affine(self) -> AffineEdge: | ||
| """Convert the transformation to an affine transformation, whenever the conversion can be made.""" |
There was a problem hiding this comment.
Up to you if doing it now or later, but I think it is going to be very beneficial to already play around with the displacement class to test code paths where a walk in the transformation graph contains a "non-affinable-transformation".
| # order of the composition: self is applied first, then the transformation passed as argument | ||
| def compose_with(self, transformation: BaseTransfEdge) -> BaseTransfEdge: | ||
| """ | ||
| Compose the transfomation object with another transformation | ||
|
|
||
| Parameters | ||
| ---------- | ||
| transformation | ||
| The transformation to compose with. | ||
|
|
||
| Returns | ||
| ------- | ||
| The compoesed transformation. | ||
|
|
||
| Notes | ||
| ------- | ||
| Self is applied first, then the transformation passed as argument. | ||
| """ | ||
| return SequenceEdge([self, transformation], name=None) # FIXME: no name? |
There was a problem hiding this comment.
I think I had this in the old NGFF classes but never used. Also, nitpick, this would lead to a transformation like [[[t0, t1], t2], t3]; which is fine, but composing in one go from outside may be more appealing [t0, t1, t2, t3].
Anyway, no big deal, with can keep.
There was a problem hiding this comment.
Regarding the name: it is optional, so we can keep it optional (or let the user pass it as optional argument to compose_with())
There was a problem hiding this comment.
I think I had this in the old NGFF classes but never used
It's ripped straight out of there =)
but composing in one go from outside may be more appealing
I'll also consider a .flatten() method
| def __repr__(self) -> str: | ||
| s = super().__repr__() + "\n" | ||
| s += "\n".join(f" {out} <- {inp}\n" for out, inp in self.output_to_input.items()) | ||
| return s |
There was a problem hiding this comment.
Rough edge: MapAxis is basically an affine (made of zeros and a few ones to permute the data). When we do repr on the affine we print the matrix and not the axes (which will be handled by the coordinate systems). Here we do the opposite, we print the axes and don't care about the affine.
It's not incorrect, but I see that this goes more towards the philosophy of spatialdata transformations, while the affine stays "true" to the NGFF transformations.
For clarify we could actually always print both: the "index-based/matrix representation" (in this case index 0 goes to 1, index 3 goes to 4 etc; in the affine case the matrix); and then the axes annotation (x -> y, y -> z here; in the case of affine the input and output axes, as we do when we print spatialdata Affine transformations).
| ) | ||
|
|
||
|
|
||
| class TranslationEdge(BaseTransfEdge): |
There was a problem hiding this comment.
Up to you if doing it here or in a follow-up PR. projectAxis is a crucial transform and we should start using it/play around with it soon.
| ) | ||
|
|
||
| def inverse(self) -> BaseTransfEdge: | ||
| inv = np.linalg.inv(self.affine) |
There was a problem hiding this comment.
worth adding a check on the stability by inspecting the conditioning number. We can reuse the code of decompose affine.
There was a problem hiding this comment.
In general inverting matrices is not a good idea, but I think the matrices coming from spatial alignments should be generally fine to be inverted.
There was a problem hiding this comment.
Would also good to catch LinAlgError and throw a custom exception with more context?
| raise ValueError("input and output should have the same numbe rof axes") | ||
| if not np.isclose(np.linalg.det(linear_matrix), 1.0): | ||
| raise ValueError("det(linear_matrix) should be ~= 1") | ||
| linear_matrix.flags.writeable = False |
There was a problem hiding this comment.
Even though self.rotation is Final, there is nothing preventing it from being modified internally (potentially through another reference that was kept outside of __init__). This flags raises an exception if anything tries writing to the array.
Still, for extra paranoia we should probably make a copy of linear_matrix so we don't accidentally freeze an array that a user might be meaning to recycle 🤔
b0ff71c to
75e5a44
Compare
* chore: added pandas-stubs as dev dependency; helps with type checking * chore: added local hatch.toml to gitignore * chore: Add .kilo and plans to .gitignore * feat: added literals and types for element types and group names * feat: added .coverage and htmlcov to gitignore * feat: transformation manager at the root of spatialdata object * fix: io_zarr.py mixup between ELEMENT_TYPE_VECTOR/RASTER * fix: avoid exposing TransformationManager in spatialdata __init__.py * fix: element associated to cs -> element belonging to cs * refac: transformation_manager now uses nx.MultiDiGraph * refac: tests to match new transformation manager implementation * fix: restored unintentionally removed functions + others fixes * feat: custom error messages for transformation graph elements * feat: custom exceptions for transform manager * refac: transform manager attributes private; added missing method * feat: more custom error and warnings + fixes * feat: more test coverage + fixes * feat: support for transformation management in graph with mutliple edges * fix: fixes in tests + renaming for readability * fix: typing Affine Transform * feat: TransformationManager return Sequence instead of list * feat: made TransformationManager.graph type specific over node type * fix: removed internal attribute access warnings * refac: rename check_if... methods to assert_... + one additional method * fix: node type spec for TransformationManager._graph * fix: made edge key definition from transforms more robust * refac: method rename * refac: TransformationManager get/remove methods don't raise error if edges are missing * feat: new test for TransformationManager.add_transformation * fix: better error messages and edge case handling for when transformation path is ambiguous * feat: TransformationManager edge (key) definition made stronger * fix: simplified access to attributes of TransformationManager * fix: TransformationManager.unset_element is now private * fix: throw error a path has one node * fix: simplified checking if coordinate system has associated transforms * fix: improved Transformation Manager code quality with better typing * fix: improved Transformation Manager exception naming and messaging * fix: improved Transformation Manager documentation * feat: added tests for Transformation Manager * fix: used explicit syntax for type definitoin * refac: typo * fix: removed TransformationManager import from spatialdata __init__.py * fix: TransformationManager, simplified code using custom error
Cleans up and modernizes the NGff classes so that they have stronger invariants and guarantee that they always have input and output. Reading and writing to zarr is done via ome-zarr-models-py. Uses graph module as part of io_raster.py::try_read_ngff06_multiscale to interpret Ngff trnasformations and produce an output that could be added to the new graph implementation.
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
84e3669 to
3a42bef
Compare
| class DeterminantDifferentFromOne(Exception): | ||
| def __init__(self, matrix: ArrayLike) -> None: | ||
| self.matrix = matrix | ||
| super().__init__("Matrix does not have det(M) == 1") | ||
|
|
||
|
|
||
| class NotOrthonormalError(Exception): | ||
| def __init__(self, matrix: ArrayLike) -> None: | ||
| self.matrix = matrix | ||
| super().__init__("Matrix is not orthonormal") |
There was a problem hiding this comment.
In these 2 exception matrix is passed but it is not used.
3 participants
Mirrors Ngff* classes in the
graphmodule, parses ome-zarr-models-py classes.CoordSystem(based onNgffCoordinateSystem)__hash__(needed for usage in graph, but we might still have the identity issue when searching for verts)*Edge(based onNgffTransformation)omz-models-pyclasses__init__and nowhere elseio_raster.py
RangeIndexto the resulting xarraysNext steps
Roundtripping
This PR only does input dfor now; The full roundtrip is showing some issues that would delay this further, so I've removed all output for now and will submit it in another PR
Using unorderd axes in coordinate systems
Since our arrays are labeled, we probably shouldn't be using a
Sequenceof axes in our coordinate systems. This will probably simplify runtime quit a lot, but might create some more work during IO