Transfo manager base by ajkswamy · Pull Request #1164 · scverse/spatialdata

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

What and Why?

https://linear.app/embl-marconato/issue/SIT-177/increment-1-transformationmanager-class-foundation

Summary

see commit messages

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.

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.

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.

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(....)

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).

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.

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.

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

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.56%. Comparing base (eb4fb3d) to head (ca80f91).

Additional details and impacted files

@@                    Coverage Diff                     @@
##           transformation_manager    #1164      +/-   ##
==========================================================
+ Coverage                   92.44%   92.56%   +0.11%     
==========================================================
  Files                          51       52       +1     
  Lines                        7820     7931     +111     
==========================================================
+ Hits                         7229     7341     +112     
+ Misses                        591      590       -1

Files with missing lines	Coverage Δ
src/spatialdata/__init__.py	`95.65% <ø> (ø)`
src/spatialdata/_core/spatialdata.py	`93.87% <100.00%> (+0.01%)`	⬆️
...atialdata/_core/transformation_manager/__init__.py	`100.00% <100.00%> (ø)`
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%> (ø)`

🚀 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.

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.

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)

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

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

Uh oh!

Conversation

ajkswamy commented Jul 14, 2026

What and Why?

Summary

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!

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!

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!

codecov Bot commented Jul 16, 2026

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!

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!

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!

Reviewers

Assignees

Tomaz-Vieira left a comment •

edited

Loading

Tomaz-Vieira Jul 14, 2026 •

edited

Loading

LucaMarconato Jul 16, 2026 •

edited

Loading

LucaMarconato commented Jul 16, 2026 •

edited

Loading