Transfo manager base#1164
Conversation
There was a problem hiding this comment.
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 =)
| """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] = {} |
There was a problem hiding this comment.
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.
|
|
||
| def __init__(self) -> None: | ||
| """Initialize a Scene with empty mappings.""" | ||
| self._coordinate_systems: dict[str, NgffCoordinateSystem] = {} |
There was a problem hiding this comment.
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.
| 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] = {} |
There was a problem hiding this comment.
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.
| associated_transformations.append(transformation_key) | ||
| return associated_transformations | ||
|
|
||
| def _get_elements_associated_with_cs(self, cs_name: str) -> list[tuple[ELEMENT_TYPE, str]]: |
There was a problem hiding this comment.
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.
| 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): |
There was a problem hiding this comment.
I would raise a different exception (or at least an exception with different text) for each situation.
|
|
||
| # Raise error if there are associated transformations or elements | ||
| if len(associated_transformations) or len(associated_elements): | ||
| raise ValueError( |
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
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.
2 participants
What and Why?
https://linear.app/embl-marconato/issue/SIT-177/increment-1-transformationmanager-class-foundation
Summary
see commit messages