Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Coordinate transformations refactoring #308

Open
LucaMarconato opened this issue Jun 22, 2023 · 3 comments
Open

Coordinate transformations refactoring #308

LucaMarconato opened this issue Jun 22, 2023 · 3 comments
Assignees

Comments

@LucaMarconato
Copy link
Member

No description provided.

@LucaMarconato LucaMarconato changed the title Transformation refactoring Coordinate transformations refactoring Jun 22, 2023
@LucaMarconato LucaMarconato self-assigned this Jun 22, 2023
@LucaMarconato LucaMarconato added this to the Transformations milestone Nov 8, 2023
@LucaMarconato
Copy link
Member Author

LucaMarconato commented Feb 1, 2024

The new coordinate transformations system is explained in this talk https://youtu.be/7HzMn_ooJmk?t=1391 from minute 23:11 to 31:45.

Pragmatically, it requires the following:

Data model

  • elements.attrs['coordinate_system'] (or maybe with the shorter name 'cs'), will be added; this will be a single 'string'
  • elements.attrs['transform'] will be removed; the information on coordinate transformation will be moved to the SpatialData` level; transformations will be defined between coordinate systems, not between elements and coordinate systems, coordinate systems and elements, nor elements and elements
  • the above changes require raster types to use xarray coordinates
  • there will be a helper function to convert back and forth between BaseTransformation (and so also NGFFBaseTransformation) and xarray coordinates. How coordinates are converted needs to be agreed on (discussion here Offset when rasterizing data #165 (comment)).
  • on disk, xarray coordinates will not be saved (this is not supported by NGFF); instead the above conversion will be used

APIs

  • there will be a helper function to transfer the coordinate transformations metadata easily between SpatialData objects. In fact, elements will not contain coordinate transformations anymore.
  • SpatialData.transform_element_to_coordinate_system() will be incorporated into SpatialData.transform_to_coordinate_system(), by the use of extra arguments. Also it will become a wrapper around transform().
  • transform(data, transformation, to_coordinate_system) will have the following behavior
    • if transformation is passed, to_coordinate_system must be None. This will modify the data in-place, and not involve/modify any coordinate system transformation. It's ok to call this function if there is only one coordinate system, but if multiple coordinate systems are being used, this usage will probably be undesirable, so we should raise a warning.
    • if transformation is None, to_coordinate_system must be provided. This will transform the data to the desired coordinate system (=change the xarray coordinates, e.g. translations/scale; and also change the data when required, e.g. rotation); also the coordinate system metadata for the element will be updated.
  • The parameter maintain_positioning must be removed, because now we can't modify one element and compensate the transformations departing from it with the inverse transformations. In fact, now the transformations will involve the whole coordinate system. We can consider adding an helper function that mimics the current behavior of maintain_positioning

File format

  • The NGFF transformation specification is not merged yet; check if we have to update the io functions to the new draft specs (or if it is merged to main by that time, implement the final version of the specs).

Release

  • We need to add deprecation notices prior to release; this refactoring will introduce some breaking changes.

CC @melonora

@LucaMarconato
Copy link
Member Author

@LucaMarconato
Copy link
Member Author

I am not sure if deepcopy() for SpatialImages will preserve the xarray coordinates, my guess it's that it will reset the xyz coordiantes (the c are passed explicitly); multiscale spatial image currently doesn't call the parser so it should be fine.

  • we should check this after this is implemented.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant