Skip to content

Implement create_slice_grid#68

Merged
constantinpape merged 21 commits intomasterfrom
slice-grid
Jun 2, 2022
Merged

Implement create_slice_grid#68
constantinpape merged 21 commits intomasterfrom
slice-grid

Conversation

@constantinpape
Copy link
Copy Markdown
Contributor

Fixes #67

Have to leave now, will finish this later today.

cc @martinschorb

if source_type != "imageSource":
raise ValueError("create_slice_grid is only supported for image sources.")

shape = _load_shape(source_data["imageData"])
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this intended to happen in original voxel space or in physical space?
My idea was to slice along physical axes (making it compatible also with any kind of transformed sources) and simply introduce translations by source.shape[axis]/ n_slices. The case of a source only being scaled to physical space and slicing happening in a source axis would then be a subset of that general slicing operation.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't quite understand what you mean. But scaling must happen in physical space, since we only get the transformed version of the source here. See line 73 and following for how the transformation is computed (and I haven't double checked if this is correct yet, since I first need to implement the infrastructure to read the shape and transformation from the source metadata (i.e. bdv.xml or ome.zarr), which might be on s3)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, makes total sense. My thinking was immersed in what the MoBIE viewer would do (that should know the bounds of the source); I completely forgot that we are in a separate code world here 😆

Obviously the shape we will build on is voxel-based an then all transformations will apply (not just scaling since all rotations and shear will also mess up the source shape).

Sorry, too much MoBIE-usage in the last day...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:) No worries

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we will need to apply all existing transformations to the "shape" vector. The slicing step would then be np.dot(T_all,shape)[axis]/n.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think applying the slicing function to a single ("mother") source would be perfectly fine. You could simply add the other sources to the grid view and its transforms later once they are generated.

This makes perfect sense, but I am not sure we need the axis parameter then. Wouldn't you always slice along the z axis? (w.r.t. the initial transformation of course)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slicing across another axis would just mean rotate it s.t. this axis becomes z

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I add another parameter to the function so that you can pass a list of transformations that's applied before the slicing?

Maybe you could even reference an existing view and "import" the sourceTransforms from there?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes perfect sense, but I am not sure we need the axis parameter then. Wouldn't you always slice along the z axis? (w.r.t. the initial transformation of course)

Yes, you would then add the extra rotation in case slicing another axis. Otherwise the grid will also not be in the xy plane. Probably referencing an existing view (that users can generate in MoBIE and export) would be indeed ideal here (and then we can ditch the axis parameter). The users probably don't care how the transforms look as long as the orientation matches with what they like to see.

Copy link
Copy Markdown
Contributor Author

@constantinpape constantinpape May 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you could even reference an existing view and "import" the sourceTransforms from there?

Ok, but I will create two functions for this, one that takes initial transforms and one that takes a reference view.
(Having both options in the same function would require some handling of precedence if both are given, and I want to avoid that)
(And the one that takes a reference view just calls the one with initial transforms internally)

Copy link
Copy Markdown
Contributor Author

@constantinpape constantinpape left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@martinschorb I refactored this a bit now and added two functions (one with, the other without reference view). I will call it a day now and tackle the actual implementation tomorrow.

write_dataset_metadata(dataset_folder, dataset_metadata)


def create_slice_grid(
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the function for creating a slice grid without passing an initial reference view.

_write_view(dataset_folder, dataset_metadata, view_file, view_name, sliced_view, overwrite)


def create_slice_grid_with_reference_view(
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the function for creating a slice grid from an initial reference view

@constantinpape
Copy link
Copy Markdown
Contributor Author

Hey @martinschorb,
I have a first implementation now, but it is completely untested, and since there are many different code paths writing all tests and debugging will take some time and I can't do all of that today.
Can you please let me know for which source you need this exactly for the paper figure? Than I will make it work for that one today and cover the rest in the next few days.

@martinschorb
Copy link
Copy Markdown
Contributor

Can you please let me know for which source you need this exactly for the paper figure?

that's tomo_38_hm. You could also leave out all transforms.
Alternatively we could do tomo38_lm including the fluorescence overlay. Then we would need the transforms from Figure2d.

@constantinpape
Copy link
Copy Markdown
Contributor Author

that's tomo_38_hm. You could also leave out all transforms.
Alternatively we could do tomo38_lm including the fluorescence overlay. Then we would need the transforms from Figure2d.

Ok, I will give it a try after lunch and ping you once I have something.

@constantinpape
Copy link
Copy Markdown
Contributor Author

Hey @martinschorb,
I have a first version that runs through and added a test view under slice-grid->slice-grid-tomo38 (on main directly).
The grid placement works, but the transformation I estimate seems to be much too small, and it shows each tomo at the same z position. I have to stop until 4 PM now, gonna have another look then.

@constantinpape constantinpape mentioned this pull request May 26, 2022
5 tasks
@constantinpape constantinpape marked this pull request as ready for review May 31, 2022 21:17
@constantinpape
Copy link
Copy Markdown
Contributor Author

Fyi @martinschorb I will probably merge this tomorrow without finishing the slice grid implementation, depending on whether we need it for the figure (might choose the continuous representation of the z-axis instead).

I still like the feature idea a lot and will implement it eventually, but I don't really have enough time right now to sit down and figure out all the transformation logic...
And I did a lot of refactoring here that I want to release asap because it significantly improves the package structure.

@constantinpape constantinpape merged commit d3fef60 into master Jun 2, 2022
@constantinpape constantinpape deleted the slice-grid branch June 2, 2022 21:39
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

Successfully merging this pull request may close these issues.

feature request: slice_volume

2 participants