Skip to content
This repository has been archived by the owner on Jan 5, 2024. It is now read-only.

Widget to transform cells to an atlas space #117

Closed
wants to merge 9 commits into from

Conversation

dstansby
Copy link
Member

@dstansby dstansby commented May 19, 2022

Fixes brainglobe/cellfinder#316, providing a plugin for users to transform their detected cells to an atlas space, where the image that they detected cells in has previously been registeterd by brainreg.

@deprecated-napari-hub-preview-bot
Copy link

deprecated-napari-hub-preview-bot bot commented May 19, 2022

Preview page for your plugin is ready here:
https://preview.napari-hub.org/brainglobe/cellfinder-napari/117
Updated: 2022-06-29T13:50:17.059457

@codecov-commenter
Copy link

codecov-commenter commented Jun 10, 2022

Codecov Report

Merging #117 (dcb3502) into main (d5c94b6) will increase coverage by 13.88%.
The diff coverage is 91.93%.

@@             Coverage Diff             @@
##             main     brainglobe/cellfinder#117       +/-   ##
===========================================
+ Coverage   81.28%   95.17%   +13.88%     
===========================================
  Files          15       19        +4     
  Lines         668      870      +202     
===========================================
+ Hits          543      828      +285     
+ Misses        125       42       -83     
Impacted Files Coverage Δ
cellfinder_napari/transform.py 85.71% <85.71%> (ø)
cellfinder_napari/tests/test_transform.py 100.00% <100.00%> (ø)
cellfinder_napari/tests/test_curation.py 100.00% <0.00%> (ø)
cellfinder_napari/tests/test_training.py 100.00% <0.00%> (ø)
cellfinder_napari/tests/test_detection.py 100.00% <0.00%> (ø)
cellfinder_napari/tests/test_train_containers.py 100.00% <0.00%> (ø)
cellfinder_napari/tests/test_utils.py 100.00% <0.00%> (ø)
cellfinder_napari/curation.py 89.83% <0.00%> (+6.77%) ⬆️
cellfinder_napari/train/train_containers.py 100.00% <0.00%> (+20.96%) ⬆️
cellfinder_napari/utils.py 93.54% <0.00%> (+29.03%) ⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d5c94b6...dcb3502. Read the comment docs.

dstansby added 3 commits June 27, 2022 16:11
Add feedback if not all layers selected

Working transformation widget
Open new plugin in example

Cast gaussian to int

[delete] add registered image layers

Update example
@dstansby dstansby marked this pull request as ready for review June 29, 2022 15:15
@dstansby dstansby requested review from adamltyson and a team June 29, 2022 15:52
Copy link
Contributor

@paddyroddy paddyroddy left a comment

Choose a reason for hiding this comment

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

A few concerns/comments on Git LFS

cellfinder_napari/tests/test_transform.py Show resolved Hide resolved
Comment on lines +64 to +67
if _DEFORMATION_FIELD_DIRECTORY is None:
reg_dir = Path(reg_meta["registration_output_folder"])
else:
reg_dir = _DEFORMATION_FIELD_DIRECTORY
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if _DEFORMATION_FIELD_DIRECTORY is None:
reg_dir = Path(reg_meta["registration_output_folder"])
else:
reg_dir = _DEFORMATION_FIELD_DIRECTORY
reg_dir = Path(reg_meta["registration_output_folder"]) if _DEFORMATION_FIELD_DIRECTORY is None else _DEFORMATION_FIELD_DIRECTORY

else:
reg_dir = _DEFORMATION_FIELD_DIRECTORY
deformation_field_paths = [
str(reg_dir / f"deformation_field_{i}.tiff") for i in [0, 1, 2]
Copy link
Contributor

Choose a reason for hiding this comment

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

I see that ultimately this is passed to tifflike.imread. Does it not accept Path objects?

@@ -34,6 +34,9 @@ jobs:
python-version: "3.9"

steps:
- uses: actions/checkout@v3
with:
lfs: true
Copy link
Contributor

@paddyroddy paddyroddy Jun 30, 2022

Choose a reason for hiding this comment

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

The actions/checkout docs are vague. I was wondering if one could specify the files manually instead of just everything tracked by Git LFS - in case we don't need all of them for the CI.

See this issue regarding my concerns on bandwidth actions/checkout#834

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for picking up on this, I wasn't aware of that issue 😃 We should probably upload the test data to a server designed for data sharing then. @adamltyson since I remember using https://gin.g-node.org in a previous PR, perhaps we should set up a new repository there for test data?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I'd rather we continued to host all large files at https://gin.g-node.org/BrainGlobe.

@@ -0,0 +1 @@
*.tiff filter=lfs diff=lfs merge=lfs -text
Copy link
Contributor

Choose a reason for hiding this comment

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

Just wondering whether a catch-all *.tiff is risky here? I think that we should be more explicit either here or in the .gitignore. Not sure if you've encountered problems with Git LFS before, but I have, and it can very quickly consume all your free quota - I've never paid for it though.

@adamltyson
Copy link
Member

Need to look into this further. Although the examples run fine, I get errors with "real" data, e.g.:

IndexError: index 356 is out of bounds for axis 1 with size 310

@adamltyson
Copy link
Member

@alessandrofelder would you be able to take a look at this sometime? It's been open for nearly a year, but I'd still like to get it merged.

The issues I remember are:

  • It fails on the real data workflows I tested it on
  • I'd like to avoid using git lfs if possible, and keep all our hosted together in one place (currently GIN)

@alessandrofelder alessandrofelder removed the request for review from adamltyson March 31, 2023 13:53
@@ -0,0 +1 @@
{"image_paths": null, "backend": "niftyreg", "voxel_sizes": ["50.0", "40.0", "40.0"], "orientation": "psl", "data_orientation": "psl", "atlas": "example_mouse_100um", "atlas_key": "atlas_key.example_mouse_100um", "registration_output_folder": "/Users/dstansby/brainreg-napari-output", "affine_n_steps": "6", "affine_use_n_steps": "5", "freeform_n_steps": "6", "freeform_use_n_steps": "4", "bending_energy_weight": "0.95", "grid_spacing": "10", "smoothing_sigma_reference": "1", "smoothing_sigma_floating": "1.0", "histogram_n_bins_floating": "128.0", "histogram_n_bins_reference": "128.0"}
Copy link
Member

Choose a reason for hiding this comment

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

Locally tests fail the assertion on Line 57 of test_transform.py

Manually emulating the test in the GUI gives:

Traceback (most recent call last):
  File "src\psygnal\_signal.py", line 950, in _run_emit_loop
  File "src\psygnal\_weak_callback.py", line 262, in cb
  File "C:\Users\Alessandro\anaconda3\envs\brainglobe\lib\site-packages\magicgui\widgets\_function_gui.py", line 218, in _disable_button_and_call
    self.__call__()
  File "C:\Users\Alessandro\anaconda3\envs\brainglobe\lib\site-packages\magicgui\widgets\_function_gui.py", line 331, in __call__
    value = self._function(*bound.args, **bound.kwargs)
  File "C:\Users\Alessandro\Documents\UCL-projects\napari-plugins\cellfinder-napari\cellfinder_napari\transform.py", line 72, in transform
    transformed_cells = transform_points_to_atlas_space(
  File "C:\Users\Alessandro\anaconda3\envs\brainglobe\lib\site-packages\cellfinder\analyse\analyse.py", line 230, in transform_points_to_atlas_space
    return transform_points_downsampled_to_atlas_space(
  File "C:\Users\Alessandro\anaconda3\envs\brainglobe\lib\site-packages\cellfinder\analyse\analyse.py", line 265, in transform_points_downsampled_to_atlas_space
    deformation_field = tifffile.imread(deformation_field_path)
  File "C:\Users\Alessandro\anaconda3\envs\brainglobe\lib\site-packages\tifffile\tifffile.py", line 1040, in imread
    with TiffFile(
  File "C:\Users\Alessandro\anaconda3\envs\brainglobe\lib\site-packages\tifffile\tifffile.py", line 3933, in __init__
    fh = FileHandle(file, mode=mode, name=name, offset=offset, size=size)
  File "C:\Users\Alessandro\anaconda3\envs\brainglobe\lib\site-packages\tifffile\tifffile.py", line 13635, in __init__
    self.open()
  File "C:\Users\Alessandro\anaconda3\envs\brainglobe\lib\site-packages\tifffile\tifffile.py", line 13650, in open
    self._fh = open(self._file, self._mode)  # type: ignore
FileNotFoundError: [Errno 2] No such file or directory: 'C:\\Users\\dstansby\\brainreg-napari-output\\deformation_field_0.tiff'

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "C:\Users\Alessandro\anaconda3\envs\brainglobe\lib\site-packages\magicgui\widgets\bases\_value_widget.py", line 65, in _on_value_change
    self.changed.emit(value)
  File "src\psygnal\_signal.py", line 905, in emit
  File "src\psygnal\_signal.py", line 952, in _run_emit_loop
psygnal._signal.EmitLoopError: calling <psygnal._weak_callback._StrongFunction object at 0x0000025851D9B5C0> with args=(False,) caused FileNotFoundError: [Errno 2] No such file or directory: 'C:\\Users\\dstansby\\brainreg-napari-output\\deformation_field_0.tiff'.

I suspect the hard-coded path here has something to do with it...
{" "registration_output_folder": "/Users/dstansby/brainreg-napari-output"}
... but how did the CI pass all that time ago? 🤔

@alessandrofelder
Copy link
Member

The error we encountered here is a consequence of brainglobe/brainglobe-workflows#42, where it should actually be fixed.

In discussion with @adamltyson we decided to discontinue the attempt at making a transform widget because

  • as it's not high-priority enough
  • what we want napari users to be able to do in this context is likely to change soon.

Therefore, closing and closing brainglobe/cellfinder#153.

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

Successfully merging this pull request may close these issues.

Support transforming detected cells to a reference atlas
5 participants