transformations (wip)

[LucaMarconato]

• io for transform
• fixed bugs from previous pull request (appeared when running on the merfish dataset from spatialdata-sandbox)

[codecov-commenter]

Codecov Report

Merging #22 (07dc775) into main (7e82f0c) will not change coverage.
The diff coverage is 0.00%.

Additional details and impacted files

@@ Coverage Diff @@## main #22 +/- ##
======================================
Coverage 0.00% 0.00% ======================================
Files 15 15 Lines 737 1343 +606 ======================================
- Misses 737 1343 +606 

Flag	Coverage Δ
unittests	0.00% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
spatialdata/_core/_spatialdata.py	0.00% <0.00%> (ø)
spatialdata/_core/coordinate_system.py	0.00% <0.00%> (ø)
spatialdata/_core/elements.py	0.00% <0.00%> (ø)
spatialdata/_core/transformations.py	0.00% <0.00%> (ø)
spatialdata/_io/format.py	0.00% <0.00%> (ø)
spatialdata/_io/read.py	0.00% <0.00%> (ø)
spatialdata/_io/write.py	0.00% <0.00%> (ø)
spatialdata/temp/bug_dask_zarr.py	0.00% <0.00%> (ø)
spatialdata/temp/bug_dask_zarr_report.py	0.00% <0.00%> (ø)
spatialdata/temp/load_and_view_merfish.py	0.00% <ø> (ø)
... and 2 more

[kevinyamauchi]

@LucaMarconato , does this need review or are you still working on this? No pressure - just checking in to see if you need anything from me.

[LucaMarconato]

Hi, there is still quite some work to be done. I am going through all the examples in spatialdata-sandbox, converting to OME-NGFF, reading them into a SpatialData object and passing them to the new napari-spatialdata plugin. You can see the current status of the datasets that work by running examples/view_spatialdata.py in napari-spatialdata.

[LucaMarconato]

@giovp there are two flake8 issues in spatialdata._constants._enum.py. I am not familiar with that code, can you check it please?

[LucaMarconato]

@giovp

I am working on this pr. Could you please give more details re this:

I think we want to have a way to store the transforms types and variables somewhere else, and in the class only materialize them. They could be as a schema or as a dataclass?> but basically I would avoid instantiating dictionaries in method call, and rather separate the transform implementations from the transforms specs

My idea is to actually keep the logic of the specs in the classes, since it is not just about data storage but also what each type of transformation can do, like an Identity can't have a ndim method telling the dimensionality since the specs don't specify information about that for identity transformations, but a ndim method is available for Rotation since the specs require the full matrix to be specified.

But what I would do is that I would separate how the specs dictate how to store the data (i.e. as json/dict) and the variables that are inside the classes. So for example Scale would now access in the constructor just a vector of scaling factors, not the corresponding dict that the NGFF specs describes.

[LucaMarconato]

@giovp ready for review! Please see the two comments regarding your previous review.

Additional comments on the pr.

1. When constructing a transformation one can decide to omit, or to specify, the input and/or output coordinate systems.
   1.1. By default they are None, and if the user don't specify them, there will be an error when trying to apply the transformation to points.
   1.2. If they are specified, then a validation step will be performed when trying to apply the transformation to points, whenever this is possible. For instance an error will be raised if one tries to apply an Identity/Translate/Scale/Rotation transformation between different input and output coordinate systems. Instead when applying a Affine transformation, only the dimensionality of the axes is checked, but the transformation will be considered valid both if the input->output are xy->xy and xy->yx. There is no way to know which of the two is correct, it is responsibility of the user.
   1.3. The coordinate system can be a string. This allows to specify a coordinate system by name. It will be responsibility of SpatialData to fill it with the appropriate CoordinateSystem object, otherwise an error will be raised when trying to apply the transformation to some points.
2. The class Sequence is complex. The reason is that by design the transformations that compose a Sequence object can be declared without specifying the input/output coordinate systems. These systems can be sometimes inferred (like if we apply a scale we know that input and output axes must coincide). Other times this is not possible. Instead of just applying the sequence of transformations without checking if the inferred coordinate systems make sense, I have written a system to infer them and check them when possible, by populating the .input_coordinate_system and .output_coordinate_system realtime when applying a Sequence transformations. These changes are then reverted back when the transformation is applied. In this way we leave the object untoched but we are able to catch bugs. I will link a discussion with John regarding this topic and add it to the comments.

[ivirshup]

from_dict

Point of feedback regarding from_dict, and an alternative to the big if ... else block:

Instead of doing one big block, I would have something more like:

TRANSFORMATION_REGISTRY= {
"Rotation": Rotation.from_dict,
...
}
deftransformation_from_dict(arg: dict) ->BaseTransformation:
returnTRANSFORMATION_REGISTRY[arg["type"]](arg)

This lets us keep code factored out, and allows registering more transformations as they're created.

[LucaMarconato]

The branch has already been merged to feature/transform_and_schema, which has been merged to main. The code review has been implemented, so no need to merge and now closing this pr.

[LucaMarconato]

• io for transform
• fixed bugs from previous pull request (appeared when running on the merfish dataset from spatialdata-sandbox)

[codecov-commenter]

Codecov Report

Merging #22 (07dc775) into main (7e82f0c) will not change coverage.
The diff coverage is 0.00%.

Additional details and impacted files

@@ Coverage Diff @@## main #22 +/- ##
======================================
Coverage 0.00% 0.00% ======================================
Files 15 15 Lines 737 1343 +606 ======================================
- Misses 737 1343 +606 

Flag	Coverage Δ
unittests	0.00% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
spatialdata/_core/_spatialdata.py	0.00% <0.00%> (ø)
spatialdata/_core/coordinate_system.py	0.00% <0.00%> (ø)
spatialdata/_core/elements.py	0.00% <0.00%> (ø)
spatialdata/_core/transformations.py	0.00% <0.00%> (ø)
spatialdata/_io/format.py	0.00% <0.00%> (ø)
spatialdata/_io/read.py	0.00% <0.00%> (ø)
spatialdata/_io/write.py	0.00% <0.00%> (ø)
spatialdata/temp/bug_dask_zarr.py	0.00% <0.00%> (ø)
spatialdata/temp/bug_dask_zarr_report.py	0.00% <0.00%> (ø)
spatialdata/temp/load_and_view_merfish.py	0.00% <ø> (ø)
... and 2 more

[kevinyamauchi]

@LucaMarconato , does this need review or are you still working on this? No pressure - just checking in to see if you need anything from me.

[LucaMarconato]

Hi, there is still quite some work to be done. I am going through all the examples in spatialdata-sandbox, converting to OME-NGFF, reading them into a SpatialData object and passing them to the new napari-spatialdata plugin. You can see the current status of the datasets that work by running examples/view_spatialdata.py in napari-spatialdata.

[LucaMarconato]

@giovp there are two flake8 issues in spatialdata._constants._enum.py. I am not familiar with that code, can you check it please?

[LucaMarconato]

@giovp

I am working on this pr. Could you please give more details re this:

I think we want to have a way to store the transforms types and variables somewhere else, and in the class only materialize them. They could be as a schema or as a dataclass?> but basically I would avoid instantiating dictionaries in method call, and rather separate the transform implementations from the transforms specs

My idea is to actually keep the logic of the specs in the classes, since it is not just about data storage but also what each type of transformation can do, like an Identity can't have a ndim method telling the dimensionality since the specs don't specify information about that for identity transformations, but a ndim method is available for Rotation since the specs require the full matrix to be specified.

But what I would do is that I would separate how the specs dictate how to store the data (i.e. as json/dict) and the variables that are inside the classes. So for example Scale would now access in the constructor just a vector of scaling factors, not the corresponding dict that the NGFF specs describes.

[LucaMarconato]

@giovp ready for review! Please see the two comments regarding your previous review.

Additional comments on the pr.

1. When constructing a transformation one can decide to omit, or to specify, the input and/or output coordinate systems.
   1.1. By default they are None, and if the user don't specify them, there will be an error when trying to apply the transformation to points.
   1.2. If they are specified, then a validation step will be performed when trying to apply the transformation to points, whenever this is possible. For instance an error will be raised if one tries to apply an Identity/Translate/Scale/Rotation transformation between different input and output coordinate systems. Instead when applying a Affine transformation, only the dimensionality of the axes is checked, but the transformation will be considered valid both if the input->output are xy->xy and xy->yx. There is no way to know which of the two is correct, it is responsibility of the user.
   1.3. The coordinate system can be a string. This allows to specify a coordinate system by name. It will be responsibility of SpatialData to fill it with the appropriate CoordinateSystem object, otherwise an error will be raised when trying to apply the transformation to some points.
2. The class Sequence is complex. The reason is that by design the transformations that compose a Sequence object can be declared without specifying the input/output coordinate systems. These systems can be sometimes inferred (like if we apply a scale we know that input and output axes must coincide). Other times this is not possible. Instead of just applying the sequence of transformations without checking if the inferred coordinate systems make sense, I have written a system to infer them and check them when possible, by populating the .input_coordinate_system and .output_coordinate_system realtime when applying a Sequence transformations. These changes are then reverted back when the transformation is applied. In this way we leave the object untoched but we are able to catch bugs. I will link a discussion with John regarding this topic and add it to the comments.

[ivirshup]

from_dict

Point of feedback regarding from_dict, and an alternative to the big if ... else block:

Instead of doing one big block, I would have something more like:

TRANSFORMATION_REGISTRY= {
"Rotation": Rotation.from_dict,
...
}
deftransformation_from_dict(arg: dict) ->BaseTransformation:
returnTRANSFORMATION_REGISTRY[arg["type"]](arg)

This lets us keep code factored out, and allows registering more transformations as they're created.

[LucaMarconato]

The branch has already been merged to feature/transform_and_schema, which has been merged to main. The code review has been implemented, so no need to merge and now closing this pr.

[LucaMarconato]

• io for transform
• fixed bugs from previous pull request (appeared when running on the merfish dataset from spatialdata-sandbox)

[codecov-commenter]

Codecov Report

Merging #22 (07dc775) into main (7e82f0c) will not change coverage.
The diff coverage is 0.00%.

Additional details and impacted files

@@ Coverage Diff @@## main #22 +/- ##
======================================
Coverage 0.00% 0.00% ======================================
Files 15 15 Lines 737 1343 +606 ======================================
- Misses 737 1343 +606 

Flag	Coverage Δ
unittests	0.00% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
spatialdata/_core/_spatialdata.py	0.00% <0.00%> (ø)
spatialdata/_core/coordinate_system.py	0.00% <0.00%> (ø)
spatialdata/_core/elements.py	0.00% <0.00%> (ø)
spatialdata/_core/transformations.py	0.00% <0.00%> (ø)
spatialdata/_io/format.py	0.00% <0.00%> (ø)
spatialdata/_io/read.py	0.00% <0.00%> (ø)
spatialdata/_io/write.py	0.00% <0.00%> (ø)
spatialdata/temp/bug_dask_zarr.py	0.00% <0.00%> (ø)
spatialdata/temp/bug_dask_zarr_report.py	0.00% <0.00%> (ø)
spatialdata/temp/load_and_view_merfish.py	0.00% <ø> (ø)
... and 2 more

[kevinyamauchi]

@LucaMarconato , does this need review or are you still working on this? No pressure - just checking in to see if you need anything from me.

[LucaMarconato]

Hi, there is still quite some work to be done. I am going through all the examples in spatialdata-sandbox, converting to OME-NGFF, reading them into a SpatialData object and passing them to the new napari-spatialdata plugin. You can see the current status of the datasets that work by running examples/view_spatialdata.py in napari-spatialdata.

[LucaMarconato]

@giovp there are two flake8 issues in spatialdata._constants._enum.py. I am not familiar with that code, can you check it please?

[LucaMarconato]

@giovp

I am working on this pr. Could you please give more details re this:

I think we want to have a way to store the transforms types and variables somewhere else, and in the class only materialize them. They could be as a schema or as a dataclass?> but basically I would avoid instantiating dictionaries in method call, and rather separate the transform implementations from the transforms specs

My idea is to actually keep the logic of the specs in the classes, since it is not just about data storage but also what each type of transformation can do, like an Identity can't have a ndim method telling the dimensionality since the specs don't specify information about that for identity transformations, but a ndim method is available for Rotation since the specs require the full matrix to be specified.

But what I would do is that I would separate how the specs dictate how to store the data (i.e. as json/dict) and the variables that are inside the classes. So for example Scale would now access in the constructor just a vector of scaling factors, not the corresponding dict that the NGFF specs describes.

[LucaMarconato]

@giovp ready for review! Please see the two comments regarding your previous review.

Additional comments on the pr.

1. When constructing a transformation one can decide to omit, or to specify, the input and/or output coordinate systems.
   1.1. By default they are None, and if the user don't specify them, there will be an error when trying to apply the transformation to points.
   1.2. If they are specified, then a validation step will be performed when trying to apply the transformation to points, whenever this is possible. For instance an error will be raised if one tries to apply an Identity/Translate/Scale/Rotation transformation between different input and output coordinate systems. Instead when applying a Affine transformation, only the dimensionality of the axes is checked, but the transformation will be considered valid both if the input->output are xy->xy and xy->yx. There is no way to know which of the two is correct, it is responsibility of the user.
   1.3. The coordinate system can be a string. This allows to specify a coordinate system by name. It will be responsibility of SpatialData to fill it with the appropriate CoordinateSystem object, otherwise an error will be raised when trying to apply the transformation to some points.
2. The class Sequence is complex. The reason is that by design the transformations that compose a Sequence object can be declared without specifying the input/output coordinate systems. These systems can be sometimes inferred (like if we apply a scale we know that input and output axes must coincide). Other times this is not possible. Instead of just applying the sequence of transformations without checking if the inferred coordinate systems make sense, I have written a system to infer them and check them when possible, by populating the .input_coordinate_system and .output_coordinate_system realtime when applying a Sequence transformations. These changes are then reverted back when the transformation is applied. In this way we leave the object untoched but we are able to catch bugs. I will link a discussion with John regarding this topic and add it to the comments.

[ivirshup]

from_dict

Point of feedback regarding from_dict, and an alternative to the big if ... else block:

Instead of doing one big block, I would have something more like:

TRANSFORMATION_REGISTRY= {
"Rotation": Rotation.from_dict,
...
}
deftransformation_from_dict(arg: dict) ->BaseTransformation:
returnTRANSFORMATION_REGISTRY[arg["type"]](arg)

This lets us keep code factored out, and allows registering more transformations as they're created.

[LucaMarconato]

The branch has already been merged to feature/transform_and_schema, which has been merged to main. The code review has been implemented, so no need to merge and now closing this pr.

[LucaMarconato]

• io for transform
• fixed bugs from previous pull request (appeared when running on the merfish dataset from spatialdata-sandbox)

[codecov-commenter]

Codecov Report

Merging #22 (07dc775) into main (7e82f0c) will not change coverage.
The diff coverage is 0.00%.

Additional details and impacted files

@@ Coverage Diff @@## main #22 +/- ##
======================================
Coverage 0.00% 0.00% ======================================
Files 15 15 Lines 737 1343 +606 ======================================
- Misses 737 1343 +606 

Flag	Coverage Δ
unittests	0.00% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
spatialdata/_core/_spatialdata.py	0.00% <0.00%> (ø)
spatialdata/_core/coordinate_system.py	0.00% <0.00%> (ø)
spatialdata/_core/elements.py	0.00% <0.00%> (ø)
spatialdata/_core/transformations.py	0.00% <0.00%> (ø)
spatialdata/_io/format.py	0.00% <0.00%> (ø)
spatialdata/_io/read.py	0.00% <0.00%> (ø)
spatialdata/_io/write.py	0.00% <0.00%> (ø)
spatialdata/temp/bug_dask_zarr.py	0.00% <0.00%> (ø)
spatialdata/temp/bug_dask_zarr_report.py	0.00% <0.00%> (ø)
spatialdata/temp/load_and_view_merfish.py	0.00% <ø> (ø)
... and 2 more

[kevinyamauchi]

@LucaMarconato , does this need review or are you still working on this? No pressure - just checking in to see if you need anything from me.

[LucaMarconato]

Hi, there is still quite some work to be done. I am going through all the examples in spatialdata-sandbox, converting to OME-NGFF, reading them into a SpatialData object and passing them to the new napari-spatialdata plugin. You can see the current status of the datasets that work by running examples/view_spatialdata.py in napari-spatialdata.

[LucaMarconato]

@giovp there are two flake8 issues in spatialdata._constants._enum.py. I am not familiar with that code, can you check it please?

[LucaMarconato]

@giovp

I am working on this pr. Could you please give more details re this:

I think we want to have a way to store the transforms types and variables somewhere else, and in the class only materialize them. They could be as a schema or as a dataclass?> but basically I would avoid instantiating dictionaries in method call, and rather separate the transform implementations from the transforms specs

My idea is to actually keep the logic of the specs in the classes, since it is not just about data storage but also what each type of transformation can do, like an Identity can't have a ndim method telling the dimensionality since the specs don't specify information about that for identity transformations, but a ndim method is available for Rotation since the specs require the full matrix to be specified.

But what I would do is that I would separate how the specs dictate how to store the data (i.e. as json/dict) and the variables that are inside the classes. So for example Scale would now access in the constructor just a vector of scaling factors, not the corresponding dict that the NGFF specs describes.

[LucaMarconato]

@giovp ready for review! Please see the two comments regarding your previous review.

Additional comments on the pr.

1. When constructing a transformation one can decide to omit, or to specify, the input and/or output coordinate systems.
   1.1. By default they are None, and if the user don't specify them, there will be an error when trying to apply the transformation to points.
   1.2. If they are specified, then a validation step will be performed when trying to apply the transformation to points, whenever this is possible. For instance an error will be raised if one tries to apply an Identity/Translate/Scale/Rotation transformation between different input and output coordinate systems. Instead when applying a Affine transformation, only the dimensionality of the axes is checked, but the transformation will be considered valid both if the input->output are xy->xy and xy->yx. There is no way to know which of the two is correct, it is responsibility of the user.
   1.3. The coordinate system can be a string. This allows to specify a coordinate system by name. It will be responsibility of SpatialData to fill it with the appropriate CoordinateSystem object, otherwise an error will be raised when trying to apply the transformation to some points.
2. The class Sequence is complex. The reason is that by design the transformations that compose a Sequence object can be declared without specifying the input/output coordinate systems. These systems can be sometimes inferred (like if we apply a scale we know that input and output axes must coincide). Other times this is not possible. Instead of just applying the sequence of transformations without checking if the inferred coordinate systems make sense, I have written a system to infer them and check them when possible, by populating the .input_coordinate_system and .output_coordinate_system realtime when applying a Sequence transformations. These changes are then reverted back when the transformation is applied. In this way we leave the object untoched but we are able to catch bugs. I will link a discussion with John regarding this topic and add it to the comments.

[ivirshup]

from_dict

Point of feedback regarding from_dict, and an alternative to the big if ... else block:

Instead of doing one big block, I would have something more like:

TRANSFORMATION_REGISTRY= {
"Rotation": Rotation.from_dict,
...
}
deftransformation_from_dict(arg: dict) ->BaseTransformation:
returnTRANSFORMATION_REGISTRY[arg["type"]](arg)

This lets us keep code factored out, and allows registering more transformations as they're created.

[LucaMarconato]

The branch has already been merged to feature/transform_and_schema, which has been merged to main. The code review has been implemented, so no need to merge and now closing this pr.

[LucaMarconato]

• io for transform
• fixed bugs from previous pull request (appeared when running on the merfish dataset from spatialdata-sandbox)

[codecov-commenter]

Codecov Report

Merging #22 (07dc775) into main (7e82f0c) will not change coverage.
The diff coverage is 0.00%.

Additional details and impacted files

@@ Coverage Diff @@## main #22 +/- ##
======================================
Coverage 0.00% 0.00% ======================================
Files 15 15 Lines 737 1343 +606 ======================================
- Misses 737 1343 +606 

Flag	Coverage Δ
unittests	0.00% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
spatialdata/_core/_spatialdata.py	0.00% <0.00%> (ø)
spatialdata/_core/coordinate_system.py	0.00% <0.00%> (ø)
spatialdata/_core/elements.py	0.00% <0.00%> (ø)
spatialdata/_core/transformations.py	0.00% <0.00%> (ø)
spatialdata/_io/format.py	0.00% <0.00%> (ø)
spatialdata/_io/read.py	0.00% <0.00%> (ø)
spatialdata/_io/write.py	0.00% <0.00%> (ø)
spatialdata/temp/bug_dask_zarr.py	0.00% <0.00%> (ø)
spatialdata/temp/bug_dask_zarr_report.py	0.00% <0.00%> (ø)
spatialdata/temp/load_and_view_merfish.py	0.00% <ø> (ø)
... and 2 more

[kevinyamauchi]

@LucaMarconato , does this need review or are you still working on this? No pressure - just checking in to see if you need anything from me.

[LucaMarconato]

Hi, there is still quite some work to be done. I am going through all the examples in spatialdata-sandbox, converting to OME-NGFF, reading them into a SpatialData object and passing them to the new napari-spatialdata plugin. You can see the current status of the datasets that work by running examples/view_spatialdata.py in napari-spatialdata.

[LucaMarconato]

@giovp there are two flake8 issues in spatialdata._constants._enum.py. I am not familiar with that code, can you check it please?

[LucaMarconato]

@giovp

I am working on this pr. Could you please give more details re this:

I think we want to have a way to store the transforms types and variables somewhere else, and in the class only materialize them. They could be as a schema or as a dataclass?> but basically I would avoid instantiating dictionaries in method call, and rather separate the transform implementations from the transforms specs

My idea is to actually keep the logic of the specs in the classes, since it is not just about data storage but also what each type of transformation can do, like an Identity can't have a ndim method telling the dimensionality since the specs don't specify information about that for identity transformations, but a ndim method is available for Rotation since the specs require the full matrix to be specified.

But what I would do is that I would separate how the specs dictate how to store the data (i.e. as json/dict) and the variables that are inside the classes. So for example Scale would now access in the constructor just a vector of scaling factors, not the corresponding dict that the NGFF specs describes.

[LucaMarconato]

@giovp ready for review! Please see the two comments regarding your previous review.

Additional comments on the pr.

1. When constructing a transformation one can decide to omit, or to specify, the input and/or output coordinate systems.
   1.1. By default they are None, and if the user don't specify them, there will be an error when trying to apply the transformation to points.
   1.2. If they are specified, then a validation step will be performed when trying to apply the transformation to points, whenever this is possible. For instance an error will be raised if one tries to apply an Identity/Translate/Scale/Rotation transformation between different input and output coordinate systems. Instead when applying a Affine transformation, only the dimensionality of the axes is checked, but the transformation will be considered valid both if the input->output are xy->xy and xy->yx. There is no way to know which of the two is correct, it is responsibility of the user.
   1.3. The coordinate system can be a string. This allows to specify a coordinate system by name. It will be responsibility of SpatialData to fill it with the appropriate CoordinateSystem object, otherwise an error will be raised when trying to apply the transformation to some points.
2. The class Sequence is complex. The reason is that by design the transformations that compose a Sequence object can be declared without specifying the input/output coordinate systems. These systems can be sometimes inferred (like if we apply a scale we know that input and output axes must coincide). Other times this is not possible. Instead of just applying the sequence of transformations without checking if the inferred coordinate systems make sense, I have written a system to infer them and check them when possible, by populating the .input_coordinate_system and .output_coordinate_system realtime when applying a Sequence transformations. These changes are then reverted back when the transformation is applied. In this way we leave the object untoched but we are able to catch bugs. I will link a discussion with John regarding this topic and add it to the comments.

[ivirshup]

from_dict

Point of feedback regarding from_dict, and an alternative to the big if ... else block:

Instead of doing one big block, I would have something more like:

TRANSFORMATION_REGISTRY= {
"Rotation": Rotation.from_dict,
...
}
deftransformation_from_dict(arg: dict) ->BaseTransformation:
returnTRANSFORMATION_REGISTRY[arg["type"]](arg)

This lets us keep code factored out, and allows registering more transformations as they're created.

[LucaMarconato]

The branch has already been merged to feature/transform_and_schema, which has been merged to main. The code review has been implemented, so no need to merge and now closing this pr.

[LucaMarconato]

• io for transform
• fixed bugs from previous pull request (appeared when running on the merfish dataset from spatialdata-sandbox)

[codecov-commenter]

Codecov Report

Merging #22 (07dc775) into main (7e82f0c) will not change coverage.
The diff coverage is 0.00%.

Additional details and impacted files

@@ Coverage Diff @@## main #22 +/- ##
======================================
Coverage 0.00% 0.00% ======================================
Files 15 15 Lines 737 1343 +606 ======================================
- Misses 737 1343 +606 

Flag	Coverage Δ
unittests	0.00% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
spatialdata/_core/_spatialdata.py	0.00% <0.00%> (ø)
spatialdata/_core/coordinate_system.py	0.00% <0.00%> (ø)
spatialdata/_core/elements.py	0.00% <0.00%> (ø)
spatialdata/_core/transformations.py	0.00% <0.00%> (ø)
spatialdata/_io/format.py	0.00% <0.00%> (ø)
spatialdata/_io/read.py	0.00% <0.00%> (ø)
spatialdata/_io/write.py	0.00% <0.00%> (ø)
spatialdata/temp/bug_dask_zarr.py	0.00% <0.00%> (ø)
spatialdata/temp/bug_dask_zarr_report.py	0.00% <0.00%> (ø)
spatialdata/temp/load_and_view_merfish.py	0.00% <ø> (ø)
... and 2 more

[kevinyamauchi]

@LucaMarconato , does this need review or are you still working on this? No pressure - just checking in to see if you need anything from me.

[LucaMarconato]

Hi, there is still quite some work to be done. I am going through all the examples in spatialdata-sandbox, converting to OME-NGFF, reading them into a SpatialData object and passing them to the new napari-spatialdata plugin. You can see the current status of the datasets that work by running examples/view_spatialdata.py in napari-spatialdata.

[LucaMarconato]

@giovp there are two flake8 issues in spatialdata._constants._enum.py. I am not familiar with that code, can you check it please?

[LucaMarconato]

@giovp

I am working on this pr. Could you please give more details re this:

I think we want to have a way to store the transforms types and variables somewhere else, and in the class only materialize them. They could be as a schema or as a dataclass?> but basically I would avoid instantiating dictionaries in method call, and rather separate the transform implementations from the transforms specs

My idea is to actually keep the logic of the specs in the classes, since it is not just about data storage but also what each type of transformation can do, like an Identity can't have a ndim method telling the dimensionality since the specs don't specify information about that for identity transformations, but a ndim method is available for Rotation since the specs require the full matrix to be specified.

But what I would do is that I would separate how the specs dictate how to store the data (i.e. as json/dict) and the variables that are inside the classes. So for example Scale would now access in the constructor just a vector of scaling factors, not the corresponding dict that the NGFF specs describes.

[LucaMarconato]

@giovp ready for review! Please see the two comments regarding your previous review.

Additional comments on the pr.

1. When constructing a transformation one can decide to omit, or to specify, the input and/or output coordinate systems.
   1.1. By default they are None, and if the user don't specify them, there will be an error when trying to apply the transformation to points.
   1.2. If they are specified, then a validation step will be performed when trying to apply the transformation to points, whenever this is possible. For instance an error will be raised if one tries to apply an Identity/Translate/Scale/Rotation transformation between different input and output coordinate systems. Instead when applying a Affine transformation, only the dimensionality of the axes is checked, but the transformation will be considered valid both if the input->output are xy->xy and xy->yx. There is no way to know which of the two is correct, it is responsibility of the user.
   1.3. The coordinate system can be a string. This allows to specify a coordinate system by name. It will be responsibility of SpatialData to fill it with the appropriate CoordinateSystem object, otherwise an error will be raised when trying to apply the transformation to some points.
2. The class Sequence is complex. The reason is that by design the transformations that compose a Sequence object can be declared without specifying the input/output coordinate systems. These systems can be sometimes inferred (like if we apply a scale we know that input and output axes must coincide). Other times this is not possible. Instead of just applying the sequence of transformations without checking if the inferred coordinate systems make sense, I have written a system to infer them and check them when possible, by populating the .input_coordinate_system and .output_coordinate_system realtime when applying a Sequence transformations. These changes are then reverted back when the transformation is applied. In this way we leave the object untoched but we are able to catch bugs. I will link a discussion with John regarding this topic and add it to the comments.

[ivirshup]

from_dict

Point of feedback regarding from_dict, and an alternative to the big if ... else block:

Instead of doing one big block, I would have something more like:

TRANSFORMATION_REGISTRY= {
"Rotation": Rotation.from_dict,
...
}
deftransformation_from_dict(arg: dict) ->BaseTransformation:
returnTRANSFORMATION_REGISTRY[arg["type"]](arg)

This lets us keep code factored out, and allows registering more transformations as they're created.

[LucaMarconato]

The branch has already been merged to feature/transform_and_schema, which has been merged to main. The code review has been implemented, so no need to merge and now closing this pr.

[LucaMarconato]

• io for transform
• fixed bugs from previous pull request (appeared when running on the merfish dataset from spatialdata-sandbox)

[codecov-commenter]

Codecov Report

Merging #22 (07dc775) into main (7e82f0c) will not change coverage.
The diff coverage is 0.00%.

Additional details and impacted files

@@ Coverage Diff @@## main #22 +/- ##
======================================
Coverage 0.00% 0.00% ======================================
Files 15 15 Lines 737 1343 +606 ======================================
- Misses 737 1343 +606 

Flag	Coverage Δ
unittests	0.00% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
spatialdata/_core/_spatialdata.py	0.00% <0.00%> (ø)
spatialdata/_core/coordinate_system.py	0.00% <0.00%> (ø)
spatialdata/_core/elements.py	0.00% <0.00%> (ø)
spatialdata/_core/transformations.py	0.00% <0.00%> (ø)
spatialdata/_io/format.py	0.00% <0.00%> (ø)
spatialdata/_io/read.py	0.00% <0.00%> (ø)
spatialdata/_io/write.py	0.00% <0.00%> (ø)
spatialdata/temp/bug_dask_zarr.py	0.00% <0.00%> (ø)
spatialdata/temp/bug_dask_zarr_report.py	0.00% <0.00%> (ø)
spatialdata/temp/load_and_view_merfish.py	0.00% <ø> (ø)
... and 2 more

[kevinyamauchi]

@LucaMarconato , does this need review or are you still working on this? No pressure - just checking in to see if you need anything from me.

[LucaMarconato]

Hi, there is still quite some work to be done. I am going through all the examples in spatialdata-sandbox, converting to OME-NGFF, reading them into a SpatialData object and passing them to the new napari-spatialdata plugin. You can see the current status of the datasets that work by running examples/view_spatialdata.py in napari-spatialdata.

[LucaMarconato]

@giovp there are two flake8 issues in spatialdata._constants._enum.py. I am not familiar with that code, can you check it please?

[LucaMarconato]

@giovp

I am working on this pr. Could you please give more details re this:

I think we want to have a way to store the transforms types and variables somewhere else, and in the class only materialize them. They could be as a schema or as a dataclass?> but basically I would avoid instantiating dictionaries in method call, and rather separate the transform implementations from the transforms specs

My idea is to actually keep the logic of the specs in the classes, since it is not just about data storage but also what each type of transformation can do, like an Identity can't have a ndim method telling the dimensionality since the specs don't specify information about that for identity transformations, but a ndim method is available for Rotation since the specs require the full matrix to be specified.

But what I would do is that I would separate how the specs dictate how to store the data (i.e. as json/dict) and the variables that are inside the classes. So for example Scale would now access in the constructor just a vector of scaling factors, not the corresponding dict that the NGFF specs describes.

[LucaMarconato]

@giovp ready for review! Please see the two comments regarding your previous review.

Additional comments on the pr.

1. When constructing a transformation one can decide to omit, or to specify, the input and/or output coordinate systems.
   1.1. By default they are None, and if the user don't specify them, there will be an error when trying to apply the transformation to points.
   1.2. If they are specified, then a validation step will be performed when trying to apply the transformation to points, whenever this is possible. For instance an error will be raised if one tries to apply an Identity/Translate/Scale/Rotation transformation between different input and output coordinate systems. Instead when applying a Affine transformation, only the dimensionality of the axes is checked, but the transformation will be considered valid both if the input->output are xy->xy and xy->yx. There is no way to know which of the two is correct, it is responsibility of the user.
   1.3. The coordinate system can be a string. This allows to specify a coordinate system by name. It will be responsibility of SpatialData to fill it with the appropriate CoordinateSystem object, otherwise an error will be raised when trying to apply the transformation to some points.
2. The class Sequence is complex. The reason is that by design the transformations that compose a Sequence object can be declared without specifying the input/output coordinate systems. These systems can be sometimes inferred (like if we apply a scale we know that input and output axes must coincide). Other times this is not possible. Instead of just applying the sequence of transformations without checking if the inferred coordinate systems make sense, I have written a system to infer them and check them when possible, by populating the .input_coordinate_system and .output_coordinate_system realtime when applying a Sequence transformations. These changes are then reverted back when the transformation is applied. In this way we leave the object untoched but we are able to catch bugs. I will link a discussion with John regarding this topic and add it to the comments.

[ivirshup]

from_dict

Point of feedback regarding from_dict, and an alternative to the big if ... else block:

Instead of doing one big block, I would have something more like:

TRANSFORMATION_REGISTRY= {
"Rotation": Rotation.from_dict,
...
}
deftransformation_from_dict(arg: dict) ->BaseTransformation:
returnTRANSFORMATION_REGISTRY[arg["type"]](arg)

This lets us keep code factored out, and allows registering more transformations as they're created.

[LucaMarconato]

The branch has already been merged to feature/transform_and_schema, which has been merged to main. The code review has been implemented, so no need to merge and now closing this pr.

[LucaMarconato]

• io for transform
• fixed bugs from previous pull request (appeared when running on the merfish dataset from spatialdata-sandbox)

[codecov-commenter]

Codecov Report

Merging #22 (07dc775) into main (7e82f0c) will not change coverage.
The diff coverage is 0.00%.

Additional details and impacted files

@@ Coverage Diff @@## main #22 +/- ##
======================================
Coverage 0.00% 0.00% ======================================
Files 15 15 Lines 737 1343 +606 ======================================
- Misses 737 1343 +606 

Flag	Coverage Δ
unittests	0.00% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
spatialdata/_core/_spatialdata.py	0.00% <0.00%> (ø)
spatialdata/_core/coordinate_system.py	0.00% <0.00%> (ø)
spatialdata/_core/elements.py	0.00% <0.00%> (ø)
spatialdata/_core/transformations.py	0.00% <0.00%> (ø)
spatialdata/_io/format.py	0.00% <0.00%> (ø)
spatialdata/_io/read.py	0.00% <0.00%> (ø)
spatialdata/_io/write.py	0.00% <0.00%> (ø)
spatialdata/temp/bug_dask_zarr.py	0.00% <0.00%> (ø)
spatialdata/temp/bug_dask_zarr_report.py	0.00% <0.00%> (ø)
spatialdata/temp/load_and_view_merfish.py	0.00% <ø> (ø)
... and 2 more

[kevinyamauchi]

@LucaMarconato , does this need review or are you still working on this? No pressure - just checking in to see if you need anything from me.

[LucaMarconato]

Hi, there is still quite some work to be done. I am going through all the examples in spatialdata-sandbox, converting to OME-NGFF, reading them into a SpatialData object and passing them to the new napari-spatialdata plugin. You can see the current status of the datasets that work by running examples/view_spatialdata.py in napari-spatialdata.

[LucaMarconato]

@giovp there are two flake8 issues in spatialdata._constants._enum.py. I am not familiar with that code, can you check it please?

[LucaMarconato]

@giovp

I am working on this pr. Could you please give more details re this:

I think we want to have a way to store the transforms types and variables somewhere else, and in the class only materialize them. They could be as a schema or as a dataclass?> but basically I would avoid instantiating dictionaries in method call, and rather separate the transform implementations from the transforms specs

My idea is to actually keep the logic of the specs in the classes, since it is not just about data storage but also what each type of transformation can do, like an Identity can't have a ndim method telling the dimensionality since the specs don't specify information about that for identity transformations, but a ndim method is available for Rotation since the specs require the full matrix to be specified.

But what I would do is that I would separate how the specs dictate how to store the data (i.e. as json/dict) and the variables that are inside the classes. So for example Scale would now access in the constructor just a vector of scaling factors, not the corresponding dict that the NGFF specs describes.

[LucaMarconato]

@giovp ready for review! Please see the two comments regarding your previous review.

Additional comments on the pr.

1. When constructing a transformation one can decide to omit, or to specify, the input and/or output coordinate systems.
   1.1. By default they are None, and if the user don't specify them, there will be an error when trying to apply the transformation to points.
   1.2. If they are specified, then a validation step will be performed when trying to apply the transformation to points, whenever this is possible. For instance an error will be raised if one tries to apply an Identity/Translate/Scale/Rotation transformation between different input and output coordinate systems. Instead when applying a Affine transformation, only the dimensionality of the axes is checked, but the transformation will be considered valid both if the input->output are xy->xy and xy->yx. There is no way to know which of the two is correct, it is responsibility of the user.
   1.3. The coordinate system can be a string. This allows to specify a coordinate system by name. It will be responsibility of SpatialData to fill it with the appropriate CoordinateSystem object, otherwise an error will be raised when trying to apply the transformation to some points.
2. The class Sequence is complex. The reason is that by design the transformations that compose a Sequence object can be declared without specifying the input/output coordinate systems. These systems can be sometimes inferred (like if we apply a scale we know that input and output axes must coincide). Other times this is not possible. Instead of just applying the sequence of transformations without checking if the inferred coordinate systems make sense, I have written a system to infer them and check them when possible, by populating the .input_coordinate_system and .output_coordinate_system realtime when applying a Sequence transformations. These changes are then reverted back when the transformation is applied. In this way we leave the object untoched but we are able to catch bugs. I will link a discussion with John regarding this topic and add it to the comments.

[ivirshup]

from_dict

Point of feedback regarding from_dict, and an alternative to the big if ... else block:

Instead of doing one big block, I would have something more like:

TRANSFORMATION_REGISTRY= {
"Rotation": Rotation.from_dict,
...
}
deftransformation_from_dict(arg: dict) ->BaseTransformation:
returnTRANSFORMATION_REGISTRY[arg["type"]](arg)

This lets us keep code factored out, and allows registering more transformations as they're created.

[LucaMarconato]

The branch has already been merged to feature/transform_and_schema, which has been merged to main. The code review has been implemented, so no need to merge and now closing this pr.