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

Make Import Spots handling single pixels and planar pixels properly #105

Merged
merged 16 commits into from
Jun 6, 2024

Conversation

stefanhahmann
Copy link
Collaborator

@stefanhahmann stefanhahmann commented May 31, 2024

This PR introduces that during the import of label images as ellipsoids some edge case are handled properly.

  1. Single Pixel case: Results now in a sphere with ~1pixel radius
  2. Planar pixels (e.g. a line or a circle): Results now in ellipsoids with semi axes of at least 0.5pixel

Import result before PR (the upper row shows circles/line that are only in the x-y plane, the lower row shows spheres):
grafik

Import result after PR (the upper row shows circles/line that are only in the x-y plane, the lower row shows spheres):
grafik

@stefanhahmann stefanhahmann self-assigned this May 31, 2024
@stefanhahmann stefanhahmann requested review from maarzt and removed request for maarzt May 31, 2024 12:30
Copy link
Collaborator

@maarzt maarzt left a comment

Choose a reason for hiding this comment

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

The PR successfully achieves the goal of supporting single pixel segments and planar segments in the "import spots from label image" plugins. How the code changes are very verbose and should be made more concise.

Additionally I noticed that the plugin produces wrong results when there is a non diagonal transform matrix in the image dataset:

image

The non diagonal transform matrix in the dataset.xml:
image

(The transform matrices are non diagonal for all dataset that use some kind of 3d registration that also allows rotation)

How can the code be made less verbose and more concise:

The three classes CircleRenderer, SphereRenderer and LineRenderer could be replaced by a single EllipsoidRenderer, which again could maybe be reduced to a private method in the LabelImageUtilsTest. This "EllipsoidRenderer" could be based on EllipsoidIterable class in Mastodon.

A line could be renderer for example using an "EllipsoidRenderer" by providing a covariance matrix with one large eigenvalue (semiaxis) and two sufficiently small eigenvalues (semiaxis).

@stefanhahmann stefanhahmann changed the title Label to ellipsoid conversion Make Import Spots handling single pixels and planar pixels properly Jun 5, 2024
@stefanhahmann stefanhahmann force-pushed the label_to_ellipsoid_conversion branch 4 times, most recently from 140be57 to 9072165 Compare June 6, 2024 11:06
@maarzt
Copy link
Collaborator

maarzt commented Jun 6, 2024

Here is dataset with a non diagonal transformation matrix that I used for my test and the screenshot above.
dataset-example.zip

@stefanhahmann stefanhahmann force-pushed the label_to_ellipsoid_conversion branch 2 times, most recently from e235bc0 to 20c5797 Compare June 6, 2024 13:13
* Since single pixels are not filtered anymore, they are now all converted to a spot in the graph
* test importing a sphere
* test importing a circle in only one plane
* test importing a single pixel
* test importing a line
* Build job had 2GB before
* Now both jobs have more consistently the same setting
…system

* Replaces scaling the center with voxel dimensions
* Replaces scaling the covariance matrix with voxel dimensions
* Required, since besides scaling, also rotation, translation and shearing may exist in the images
@stefanhahmann stefanhahmann force-pushed the label_to_ellipsoid_conversion branch from 20c5797 to 22302a3 Compare June 6, 2024 13:14
Copy link

sonarcloud bot commented Jun 6, 2024

@stefanhahmann
Copy link
Collaborator Author

Additionally I noticed that the plugin produces wrong results when there is a non diagonal transform matrix in the image dataset:

(The transform matrices are non diagonal for all dataset that use some kind of 3d registration that also allows rotation)

Thanks for noticing this. The transform matrices are taken into account via 9e77262

How can the code be made less verbose and more concise:

The three classes CircleRenderer, SphereRenderer and LineRenderer could be replaced by a single EllipsoidRenderer, which again could maybe be reduced to a private method in the LabelImageUtilsTest. This "EllipsoidRenderer" could be based on EllipsoidIterable class in Mastodon.

A line could be renderer for example using an "EllipsoidRenderer" by providing a covariance matrix with one large eigenvalue (semiaxis) and two sufficiently small eigenvalues (semiaxis).

Thanks for this idea. I introduced a class SpotRenderer that is able to render spheres, circles and lines using the EllipsoidIterable. However, this approach brings some difficulties, since the EllipsoidIterable inflate the ellipsoid by half a pixel, which makes it harder to use in the unit tests, since the results are less obvious. I thus kept the original renderers

@stefanhahmann stefanhahmann merged commit dd36086 into master Jun 6, 2024
3 checks passed
@stefanhahmann stefanhahmann deleted the label_to_ellipsoid_conversion branch June 6, 2024 14:27
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.

2 participants