Transformation manager foundation by ajkswamy · Pull Request #1164 · scverse/spatialdata

ajkswamy · 2026-07-14T15:29:53Z

Part of tbd

Increment 1: TransformationManager Class Foundation

Implement TransformationManager with coordinate system, element, and transformation management.
Validate against duplicate names and ensure elements belong to a CS before registration (i.e. create if not specified).
Add transformation_manager: TransformationManager as an attributed to SpatialData (not yet used).
This delivers the core data model and API for centralized transformation storage,
enabling inspection of transformation graphs without changing any existing behavior.

Proposed Architecture

TransformationManager Internal Model

import networkx as nx


class TransformationManager:
    def __init__(self):
        self.graph: nx.MultiDiGraph
        # has coordinate systems as `NgffCoordinateSystem` objects as nodes;
        # transforms as instances of BaseTransform are passed as attributes while adding edges
        self.element_to_cs_mapping: dict[str, NgffCoordinateSystem] = {}
        # maps `element_name` to the coordinate system to which the element belongs.

Notes:

attributes of TransformationManager should be private (propery pattern)
element_name is unique across all elements of a spatialdata object

TransformationManager API

Coordinate System Management

add_coordinate_system(cs: NgffCoordinateSystem) -> None
- Register a new coordinate system.
- Make sure to throw error if coordinate system already exists
remove_coordinate_system(cs: NgffCoordinateSystem) -> None:
- Remove an existing coordinate system.
- Throw error if coordinate system has not yet been added
list_coordinate_systems() -> list[NgffCoordinateSystem]:
- List all registered coordinate systems.

Element Management

add_element(element_name: str) -> None:
- Register an element to an existing coordinate system
- throw warning and do nothing if the element has already been registered
unset_element(element_name: str) -> None:
- Unregister an element from the coordinate system to which it belongs
- throw warning and do nothing if element has not been registered to any coordinate system
get_element_coordinate_system(element_name: str) -> NgffCoordinateSystem:
- Get the name of the coordinate system to which an element belongs.
- Throw error if element_name doesn't belong to any existing coordinate system

Transformation Management

add_transformation(input_cs: NgffCoordinateSystem, output_cs: NgffCoordinateSystem, transformation: BaseTransformation) -> None:
Add a transformation between coordinate systems.
throw error if either coordinate systems have not been added
get_existing_transformation(input_cs: NgffCoordinateSystem, output_cs: NgffCoordinateSystem) -> BaseTransformation | None:
Retrieve a transformation between coordinate systems.
throw error if either coordinate systems have not been added or if there is no direct transformation defined between
them
remove_transformation(input_cs: NgffCoordinateSystem, output_cs: NgffCoordinateSystem) -> None:
- Remove a transformation between coordinate systems.
- throw error if either coordinate systems have not been added

Tomaz-Vieira

I left a bunch of mostly typing-related comments, as you might imagine 😆

Also, this is something we might need to decide as a team, but it's a bit funny that often the docstring is way longer than the code itself, and often there isn't much more to say, like:

souce_cs
    The name of the source coordinate system

I often feel like the typing signature is more concise and more informative than the docstrings, and that sometimes we could survive just with the headline and the "raises" part.

But anyway, good stuff, it's nice to see we're moving already =)

Tomaz-Vieira · 2026-07-14T16:04:16Z

+        """Initialize a Scene with empty mappings."""
+        self._coordinate_systems: dict[str, NgffCoordinateSystem] = {}
+        # mapping names of coordinate system to coordinate systems
+        self._coordinate_transforms: dict[tuple[str, str], BaseTransformation] = {}


This likely should be a graph so we can find indirect paths between coordinate systems, and also so we could have multiple different edges between the same 2 CSs. I see below that you've made nx into an optional dependency, but it seems to me like we might need it as a hard dependency.

Since these are "private" variables I'm fine with both. Either we construct a nx object on the fly each time, or we use this directly for the graph representation. In both cases, the internal variables are used to represent a graph.

Implemented internal nx.MultiDiGraph

Tomaz-Vieira · 2026-07-14T16:06:03Z

+
+    def __init__(self) -> None:
+        """Initialize a Scene with empty mappings."""
+        self._coordinate_systems: dict[str, NgffCoordinateSystem] = {}


I wonder if this extra level of indirection (i.e. CS name -> CS) is the best approach. I makes our APIs "stringly" typed, where they take and return str (like in _get_transformations_associated_with_cs) rather than working with something more semantically clear, which is the NgffCoordinateSystem itself.

Added the comment also somewhere, writing it also here: since transformation manager is used internally let's do what is most convenient and clearer for us. I agree that using directly the CS objects instead of str is more robust. For user facing API calls we need to think it through, since some users come from single-cell and they have no experience (and maybe no interests) in working with NGFF classes. In those cases, passing the name of the cs would lead to the same plots/results than the full CS object.

Graph nodes are now NgffCoordinateSystem and edges contain spatialdata transforms as attributes. In addition repr of spatialdata transformations as "edge keys", to be able to distinguish for the case when there are multiple edges between nodes

Tomaz-Vieira · 2026-07-14T16:08:31Z

+        self._coordinate_transforms: dict[tuple[str, str], BaseTransformation] = {}
+        # mapping a tuple of coordinate system names, (<source coordinate system name>, <target coordinate system name>)
+        # to a transformation object representing the transformation between them
+        self._element_to_cs_mapping: dict[tuple[ELEMENT_TYPE, str], str] = {}


I'll be sounding like a broken record, but the fact that we use strings for everything here (CS name and Element name) is a bit confusing. It seems we could just straight up have a

dict[ELEMENT_TYPE, NgffCoordinateSystem]

which reads easier and type checks more tightly, since there can never be mixups between element names and CS names. We do have to make sure that all elements have reasonable __hash__ and __eq__, though.

@ajkswamy in this case we don't need to have a tuple of 2 items because the element_name is guaranteed to be unique across the object, and also because the only reason we have element_type is legacy: we choose to put all the points in a folder, all the images in a different folders, etc. and in the past there was no uniqueness across these different folders. But this on-disk structure makes NGFF interop more difficult, because any arbitrary OME-Zarr store with images and labels doesn't contain the 'images/my_image', 'labels/my_labels' folder naming convention. So long story short, we were thinking of making the spatialdata object fully flat on disk, and use only the element name (indepedently of the type) to refer to elements.

Curerntly during IO we have helper functions like _get_element_type_from_element_name(), so we need both element_type and element_name, but once we move to a flat storage, element_type would not be needed anymore.

For internal variables like this we can do what is most convenient for us (for instance passing the element object instead of the element name), but I would prefer to avoid having the user having ever to instantiate a NgffCoordinateSystem unless they really want to. For most users a str would do the job well. Let's keep this in mind in the context of the user-facing APIs and then make a decision.

done, we have self.element_to_cs_mapping: dict[str, NgffCoordinateSystem] = {}

Tomaz-Vieira · 2026-07-14T16:15:39Z

+                associated_transformations.append(transformation_key)
+        return associated_transformations
+
+    def _get_elements_associated_with_cs(self, cs_name: str) -> list[tuple[ELEMENT_TYPE, str]]:


Maybe we need a word that is better than "associated" to mean that the element belongs or exists in the coordinate system. It's a stronger relation than the transforms that are "associated" with the coordinate systems, because an element can only belong so a single CS, but a CS can have many transforms.

I agree, I like belongs for instance, but no hard opinions.

How about "element set in a coordinate system"? the API would be like

class TransformationManager:

__ init__

.......

........

set_element_in_coordinate_system(....)

unset_element_from_coordinate_system(....)

get _element_coordinate_system (....) "Get the coordinate system in which an element is set"

get_elements_set_in_cs(....)

Currently, I have tried to stick to "transformations associated with a cs" and "elements belonging to a cs". Only exception is "unset_element" for which I don't have a better idea. Any ideas?

Tomaz-Vieira · 2026-07-14T16:18:33Z

+        associated_elements = self._get_elements_associated_with_cs(cs_name)
+
+        # Raise error if there are associated transformations or elements
+        if len(associated_transformations) or len(associated_elements):


I would raise a different exception (or at least an exception with different text) for each situation.

I agree. Around the codebase we use standard exceptions with different text each time. I will comment on standard exceptions vs custom spatialdata exceptions below (where you talk above that).

Done, only custom exceptions and warning used

Tomaz-Vieira · 2026-07-14T16:20:35Z

+
+        # Raise error if there are associated transformations or elements
+        if len(associated_transformations) or len(associated_elements):
+            raise ValueError(


Personally, I like declaring custom exceptions so that users can catch them explicitly, A ValueError is very general and doesn't say much about the kind of failure that happened. If a user wants to try removing a CS and fails, it would be nice if they could except CoordSystemInUse or except CoordSystemMissing to handle the different cases.

I agree. In the codebase we use standard exception and with catch them by comparing the string, but a cleaner approach would be to define custom exception. We could use custom exceptions here, open an issue to track that at some point we should use custom exceptions elsewhere, and do the transition gradually. If we are going to do the large refactoring, we need to ensure that we subclass the existing exceptions because we don't want to break already implemented try except blocks from user libraries.

Done, only custom exceptions and warning used

Tomaz-Vieira · 2026-07-14T16:32:35Z

+            raise KeyError(f"Transformation from '{input_cs}' to '{output_cs}' not found.")
+        del self._coordinate_transforms[key]
+
+    def build_nx_graph(self) -> Any:  # type: ignore[unresolved-reference]  # noqa: F821


Maybe we could some shenanigans like

if TYPE_CHECKING: import networkx as nx ... def build_nx_graph(self) -> "nx.DiGraph": import networkx as nx # it's unfortunate that we need to import again here, though =/ ...

But I suspect we'd need a hard dependency on this anyway to be compatible with all ways in which ngff can transform.

The reason why we do local import was that we found out using profimp that the import time of nx appeared to be slow. But otherwise, hard dependency is the way. We can actually move again to global import and we rebenchmark the import time after we are done with the refactoring. The biggest culprit is dask.

Using global import for now

codecov · 2026-07-16T13:10:24Z

Codecov Report

❌ Patch coverage is 98.87640% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.81%. Comparing base (eb4fb3d) to head (3692729).

Files with missing lines	Patch %	Lines
...atialdata/_core/transformation_manager/__init__.py	98.82%	2 Missing ⚠️
...ialdata/_core/transformation_manager/exceptions.py	98.36%	1 Missing ⚠️

Additional details and impacted files

@@                    Coverage Diff                     @@
##           transformation_manager    #1164      +/-   ##
==========================================================
+ Coverage                   92.44%   92.81%   +0.36%     
==========================================================
  Files                          51       53       +2     
  Lines                        7820     8068     +248     
==========================================================
+ Hits                         7229     7488     +259     
+ Misses                        591      580      -11

Files with missing lines	Coverage Δ
src/spatialdata/__init__.py	`95.65% <ø> (ø)`
src/spatialdata/_core/spatialdata.py	`93.87% <100.00%> (+0.01%)`	⬆️
src/spatialdata/_io/io_raster.py	`89.52% <100.00%> (+0.47%)`	⬆️
src/spatialdata/_io/io_zarr.py	`92.38% <100.00%> (ø)`
src/spatialdata/_types.py	`100.00% <100.00%> (ø)`
src/spatialdata/_utils.py	`85.52% <100.00%> (ø)`
src/spatialdata/transformations/transformations.py	`93.18% <ø> (+2.09%)`	⬆️
...ialdata/_core/transformation_manager/exceptions.py	`98.36% <98.36%> (ø)`
...atialdata/_core/transformation_manager/__init__.py	`98.82% <98.82%> (ø)`

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

LucaMarconato · 2026-07-16T13:33:25Z

(to be moved to a separate issue)
We can probably restore networkx as a global import, as you can see from running profimp, dask.dataframe is what is driving the slow import time.

Importing networkx is actually 273 ms.

LucaMarconato · 2026-07-16T13:41:53Z

+        """
+        return list(self._coordinate_systems.keys())
+
+    def add_element(self, element_type: ELEMENT_TYPE, element_name: str, coordinate_system: str) -> None:


Reminder: belong vs associated to be made consistent. And also element_type here is redundant.

Done (see related comment thread)

LucaMarconato · 2026-07-16T13:43:28Z

+    def get_existing_transformation(self, input_cs: str, output_cs: str) -> BaseTransformation | None:
+        """
+        Retrieve a transformation defined between coordinate systems.
+
+        Parameters
+        ----------
+        input_cs


TODO: check if the specs allows multi edges between the coordinate systems. There was some discussion regarding this at the Heidelberg hackathon and I don't remmber the outcome.

I don't see anything in the specification that forbids multiple edges between coordinate systems. Also, transformnd uses nx.MultiDiGraph and does not check if an edge already exists between two nodes when adding edges to graph.

LucaMarconato · 2026-07-16T13:44:42Z

+        key = (input_cs, output_cs)
+        return self._coordinate_transforms.get(key)
+
+    def remove_transformation(self, input_cs: str, output_cs: str) -> None:


if we have a multigraph, we need to change the signature to let the user remove a specific transformation.

LucaMarconato · 2026-07-16T13:47:28Z

+            g.add_edge(input_cs, output_cs, transformation=transformation)
+        return g
+
+    def get_shortest_transformation_sequence(self, source_cs: str, target_cs: str) -> list[BaseTransformation]:


We will sometimes have multiple shortest paths between 2 coordinate systems (this may happen even if we don't have a multigraph). What we do in spatialdata is shown here. It's not the cleanest approach, but it could be used also here to get the job done:

spatialdata/src/spatialdata/transformations/operations.py

Line 218 in eb4fb3d

intermediate_coordinate_systems: SpatialElement | str | None = None,

. Note one can still construct edge cases that fail, the clean approach may be to pass the full list of cs as a path.

Thanks for the idea Luca. To solve this issue, I have implemented something more in line with what networkx does in such cases, which is to return all shortest paths and let the caller decide (get_all_shortest_transformation_sequences). Might be okay as these are internal APIs

For the cases where there are multiple edges between a pair of nodes in a path, then an error is raised by default ("TransformationPathAmbiguousError"). Caller can specify which of the multiple edges is to be selected by passing the corresponding transform into the list argument "expected_intermediate_transformations"

The same logic for disambiguation also applies to get_all_transformation_sequences

LucaMarconato · 2026-07-16T13:48:14Z

+            transformations.append(g[path[i]][path[i + 1]]["transformation"])
+        return transformations
+
+    def get_all_transformation_sequences(self, source_cs: str, target_cs: str) -> list[list[BaseTransformation]]:


We could return directly a list[spatialdata.transformation.Sequence]

Ideas:

@ajkswamy suggested to let the user choose the return type list of list vs list of Sequence. This has the con of complicating typing.

Comments:

Pro: of returning a list of list: the user can easily extract sub transformations

Con: if you have a Sequence you can directly use it to get an affine, to transform things. At the cost that you have to call my_sequence.transformations to get the subcomponents (but it's not that bad)

done, returning list[spatialdata.transformation.Sequence]

LucaMarconato · 2026-07-16T13:51:40Z

+            all_sequences.append(sequence)
+        return all_sequences
+
+    def __repr__(self) -> str:


For later: it would be very nice to let the user be able to plot this. An option could be a hard coded HTML rendering with no additional dependencies, but this comes with extra code to maintain.

If the user already import matplotlib (via spatialdata-plot), we could plot via networkx + matplotlib.

LucaMarconato · 2026-07-16T13:57:23Z

    "bounding_box_query": "spatialdata._core.query.spatial_query",
    "polygon_query": "spatialdata._core.query.spatial_query",
+    # _core.transformation_manager
+    "TransformationManager": "spatialdata._core.transformation_manager",


No need for the user to instantiate it. It can be removed.

LucaMarconato · 2026-07-16T13:57:35Z

    "bounding_box_query",
    "polygon_query",
+    # _core.transformation_manager
+    "TransformationManager",


Same here, to be removed.

LucaMarconato · 2026-07-16T13:57:50Z

+    # _core.transformation_manager
+    from spatialdata._core.transformation_manager import TransformationManager


ajkswamy added 6 commits July 10, 2026 11:29

chore: added pandas-stubs as dev dependency; helps with type checking

30511a7

chore: added local hatch.toml to gitignore

986f0bb

chore: Add .kilo and plans to .gitignore

5e0d12f

feat: added literals and types for element types and group names

81bb674

feat: added .coverage and htmlcov to gitignore

787770a

feat: transformation manager at the root of spatialdata object

9b2d259

Tomaz-Vieira reviewed Jul 14, 2026

View reviewed changes

fix: io_zarr.py mixup between ELEMENT_TYPE_VECTOR/RASTER

ca80f91

LucaMarconato reviewed Jul 16, 2026

View reviewed changes

ajkswamy added 10 commits July 16, 2026 16:59

fix: avoid exposing TransformationManager in spatialdata __init__.py

8f9d07f

fix: element associated to cs -> element belonging to cs

dce3dc6

refac: transformation_manager now uses nx.MultiDiGraph

367d99e

refac: tests to match new transformation manager implementation

65aadb6

fix: restored unintentionally removed functions + others fixes

611abee

feat: custom error messages for transformation graph elements

fd042a6

feat: custom exceptions for transform manager

070db93

refac: transform manager attributes private; added missing method

5314e02

feat: more custom error and warnings + fixes

5eadef4

feat: more test coverage + fixes

ab98633

ajkswamy self-assigned this Jul 21, 2026

ajkswamy added 4 commits July 22, 2026 14:14

feat: support for transformation management in graph with mutliple edges

ea28e55

fix: fixes in tests + renaming for readability

932703c

fix: typing Affine Transform

7658a19

feat: TransformationManager return Sequence instead of list

3692729

ajkswamy marked this pull request as ready for review July 23, 2026 09:16

ajkswamy requested review from LucaMarconato and Tomaz-Vieira July 23, 2026 09:16

ajkswamy changed the title ~~Transfo manager base~~ Transformation manager foundation Jul 23, 2026

		# _core.transformation_manager
		from spatialdata._core.transformation_manager import TransformationManager

Uh oh!

Conversation

ajkswamy commented Jul 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Increment 1: TransformationManager Class Foundation

Proposed Architecture

TransformationManager Internal Model

Notes:

TransformationManager API

Coordinate System Management

Element Management

Transformation Management

Uh oh!

Tomaz-Vieira left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajkswamy Jul 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tomaz-Vieira Jul 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LucaMarconato Jul 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Jul 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

LucaMarconato commented Jul 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajkswamy commented Jul 14, 2026 •

edited

Loading

Tomaz-Vieira left a comment •

edited

Loading

ajkswamy Jul 21, 2026 •

edited

Loading

Tomaz-Vieira Jul 14, 2026 •

edited

Loading

LucaMarconato Jul 16, 2026 •

edited

Loading

codecov Bot commented Jul 16, 2026 •

edited

Loading

LucaMarconato commented Jul 16, 2026 •

edited

Loading

ajkswamy Jul 21, 2026 •

edited

Loading