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

Add functionality to save stitched image as h5 or zarr #14

Open
wants to merge 23 commits into
base: main
Choose a base branch
from

Conversation

IgorTatarnikov
Copy link
Member

@IgorTatarnikov IgorTatarnikov commented Sep 5, 2024

Description

What is this PR

  • Bug fix
  • Addition of a new feature
  • Other

Why is this PR needed?
There is no way to save the fused image in the preview as one 3D image.

What does this PR do?
Adds functionality to save the fused image as either a BigDataViewer compatible h5 file or as an OME-Zarr.

How has this PR been tested?

Tests have been added to cover the new functionality.

NOTE:
To test the functionality locally you must install Fiji and also add BigStitcher to the update sites. This can be done using the GUI see here or via the command line after installing Fiji.

./ImageJ-linux64 --update add-update-sites "BigStitcherUpdate" "https://sites.imagej.net/BigStitcher/"
./ImageJ-linux64 --update update

Replace ImageJ-linux64 with the path to the local ImageJ executable.

Three different datasets are available for testing:

  1. Tiny (~28 MB): this is mostly used for automated testing and doesn't work for visual inspection. It can be downloaded here.
  2. Small (~1.8 GB): this dataset is more useful for visual inspection. It can be downloaded here.
  3. A full size dataset is available (~130 GB): this dataset works best for testing overall performance, especially when reading or writing data. It's available by request.

Is this a breaking change?

No.

Does this PR require an update to the documentation?

No.

Checklist:

  • The code has been tested locally
  • Tests have been added to cover all new functionality (unit & integration)
  • The code has been formatted with pre-commit

Copy link

codecov bot commented Sep 5, 2024

Codecov Report

Attention: Patch coverage is 91.84549% with 19 lines in your changes missing coverage. Please review.

Project coverage is 95.20%. Comparing base (58db8aa) to head (5e55376).

Files with missing lines Patch % Lines
brainglobe_stitch/stitching_widget.py 79.51% 17 Missing ⚠️
brainglobe_stitch/image_mosaic.py 98.66% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #14      +/-   ##
==========================================
- Coverage   96.89%   95.20%   -1.69%     
==========================================
  Files           6        6              
  Lines         419      626     +207     
==========================================
+ Hits          406      596     +190     
- Misses         13       30      +17     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@IgorTatarnikov IgorTatarnikov marked this pull request as ready for review September 10, 2024 12:30
Copy link

@JoeZiminski JoeZiminski left a comment

Choose a reason for hiding this comment

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

Hey Igor this looks / works great! It ran nicely on the big_test_data and was easy to use. The code was clear to read, I'm not so familiar with zarr / h5 so could not pass an expert eye over everything but looks good to me. I've got some questions / suggestions on the code and below is some feedback from using the GUI:

  • Once 'Fuse' is run, does it make sense to auto-load it into napari? Or maybe a popup like 'fused file saved to XXXX'. I wasn't sure what had happened once 'fused' was pressed. However, later learnt to keep an eye on the terminal when using napari. But maybe new users are not familiar with this and using it as a pure GUI.
  • When you click the 'Browse' button, even if you don't select anything it overwrites anything you have in the lineedit. Sometimes I miss-clicked on it and it removed what I had in the lineedit when I didn't want it to. Maybe it is worth checking if the file browser dialog returns "" or whatever it's null return is, and in this case do not update the lineedit.
  • I ran this on the very tiny dataset and got an error, might be worth looking into (but maybe just a small dataset issue)
  • If I tried to save the fuse output to a folder that already exists, I get a relative uninformative error path '0' contains an array.

.github/workflows/test_and_deploy.yml Outdated Show resolved Hide resolved
brainglobe_stitch/file_utils.py Outdated Show resolved Hide resolved
brainglobe_stitch/file_utils.py Outdated Show resolved Hide resolved
brainglobe_stitch/image_mosaic.py Show resolved Hide resolved
brainglobe_stitch/image_mosaic.py Outdated Show resolved Hide resolved
brainglobe_stitch/file_utils.py Outdated Show resolved Hide resolved
ET.indent(tree, space=" ")
tree.write(output_xml_path, encoding="utf-8", xml_declaration=True)

return

Choose a reason for hiding this comment

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

I am not very familiar with this setup but this looks good to me. Is it worth adding a test that all elements are read and assigned correctly? maybe this is overkill

Copy link
Member Author

Choose a reason for hiding this comment

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

Shouldn't be too hard, I think I have all of the expected values stored.

brainglobe_stitch/stitching_widget.py Show resolved Hide resolved
brainglobe_stitch/image_mosaic.py Outdated Show resolved Hide resolved
brainglobe_stitch/image_mosaic.py Outdated Show resolved Hide resolved
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