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 features from cellmap fork #4

Merged
merged 31 commits into from
Mar 27, 2024
Merged

Conversation

d-v-b
Copy link

@d-v-b d-v-b commented Feb 13, 2024

@rhoadesScholar can you summarize the changes from the cellmap fork?

@rhoadesScholar
Copy link
Contributor

changes were made by @yuriyzubov, so he'd probably give the best summary.

@mzouink
Copy link
Contributor

mzouink commented Mar 25, 2024

@yuriyzubov can you please explain what you did ?

@rhoadesScholar
Copy link
Contributor

These changes facilitate use of funlib.persistence with multi-resolution OME-NGFF Zarr containers, as used by CellMap. This is a core dependency to DaCapo and needs to be updated on PyPi before DaCapo can be published.

@yuriyzubov Are there any further additions you would like to make?

@yuriyzubov
Copy link
Contributor

  1. For _read_voxel_size_offset method:

    1. The store (path to the zarr/n5 container) and path (location of the array within zarr/n5 container) of the input zarr/n5 array is regularized with separate_store_path() method
    2. For zarr container, multiscale is not always located in the parent group metadata of the zarr array, so check_for_multiscale() traverses through all parent groups of the array until it finds a multiscale attribute.
    3. The information about scale is not complete without units, thus search for units is realized with check_for_units().
  2. For zarr container, when calling prepare_ds() method, adding multiscale attribute to parent group metadata is realized through add_multiscales_metadata() method.

@mzouink
Copy link
Contributor

mzouink commented Mar 25, 2024

@yuriyzubov @d-v-b Can you please take a look here
data :
/nrs/cellmap/data/jrc_mus-liver-zon-1/jrc_mus-liver-zon-1.n5/em/fibsem-uint8/
/nrs/cellmap/zouinkhim/data/tmp_data_v3/jrc_mus-liver-zon-1/jrc_mus-liver-zon-1.n5/volumes/groundtruth/crop345/labels/mito

After the PR the axes are flipped for GT in N5
Screenshot 2024-03-25 at 12 59 25 PM
Screenshot 2024-03-25 at 12 57 58 PM

@d-v-b
Copy link
Author

d-v-b commented Mar 25, 2024

I can confirm that /nrs/cellmap/zouinkhim/data/tmp_data_v3/jrc_mus-liver-zon-1/jrc_mus-liver-zon-1.n5/volumes/groundtruth/crop345/labels/mito is not aligned to the FIB-SEM data, but I need to see the code you used to generate that image in order to diagnose the issue.

@rhoadesScholar rhoadesScholar requested review from rhoadesScholar and removed request for cmalinmayor March 25, 2024 20:42
@d-v-b d-v-b removed their assignment Mar 25, 2024
@d-v-b
Copy link
Author

d-v-b commented Mar 25, 2024

@rhoadesScholar I unassigned myself, because it's not clear what you need from me in this PR

@rhoadesScholar
Copy link
Contributor

rhoadesScholar commented Mar 25, 2024

@rhoadesScholar I unassigned myself, because it's not clear what you need from me in this PR

I assigned you, @d-v-b , because you created the PR and pair well with @yuriyzubov to complete remaining issues. After that I'll merge it. Thanks!

@d-v-b
Copy link
Author

d-v-b commented Mar 25, 2024

complete remaining issues.

maybe list these issues here? I don't know what they are :)

@d-v-b
Copy link
Author

d-v-b commented Mar 25, 2024

to be clear, I opened the PR because someone needs to open a PR to bring the cellmap stuff into this repo, but none of the commits in here are mine 🤣, and I'm not a maintainer of this repo

@mzouink
Copy link
Contributor

mzouink commented Mar 25, 2024

The remaining issue:
Using the current version of funlib.persistence the N5 datasets aligns togethers but using the PR version they don't align anymore. Seams like default offset order for N5 is wrong

@rhoadesScholar rhoadesScholar merged commit 7f8ce63 into funkelab:main Mar 27, 2024
2 checks passed
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.

4 participants