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

rename-wells CLI utility #232

Merged
merged 29 commits into from
Oct 3, 2024
Merged

Conversation

josephschull
Copy link
Contributor

@josephschull josephschull commented Jul 7, 2024

This PR adds:

  • a rename-wells utility with signature iohub rename-wells -i input.zarr -c well-mapping.csv, which renames the input well names in place. The well-mapping.csv file has two columns with old and new names, and I have an example well-mapping.csv in /iohub/docs/examples
  • an API to rename single wells via ngff.rename_well. The CLI uses this API.

@ieivanov
Copy link
Contributor

ieivanov commented Jul 8, 2024

Hey @josephschull @talonchandler, I'd like to test this PR - let me know if it's ready. I have a CSV like this in mind: /hpc/projects/intracellular_dashboard/viral-sensor/2024_06_13_SEC61_TOMM20_ZIKV_DENGUE_1/0-convert/well_map.csv but I can modify it as it makes sense

@ziw-liu ziw-liu added enhancement New feature or request NGFF OME-NGFF (OME-Zarr format) labels Jul 8, 2024
Copy link
Contributor

@talonchandler talonchandler left a comment

Choose a reason for hiding this comment

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

Hey Joseph, I've made some suggestions. Let's test this (manually first, then with simple automated tests) before we request further input from Ivan and/or Ziwen.

iohub/cli/cli.py Outdated Show resolved Hide resolved
iohub/cli/cli.py Outdated Show resolved Hide resolved
iohub/cli/cli.py Outdated Show resolved Hide resolved
iohub/cli/cli.py Outdated Show resolved Hide resolved
iohub/ngff.py Outdated Show resolved Hide resolved
iohub/cli/cli.py Outdated Show resolved Hide resolved
iohub/ngff.py Outdated Show resolved Hide resolved
iohub/ngff.py Outdated Show resolved Hide resolved
iohub/cli/cli.py Outdated Show resolved Hide resolved
@talonchandler talonchandler changed the title Updated rename wells functionality at CLI level, fixed style errors rename-wells CLI utility Jul 17, 2024
Copy link
Contributor

@talonchandler talonchandler left a comment

Choose a reason for hiding this comment

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

I just tried renaming with the following file:

0/0,A/1
0/1,B/1
0/2,C/1

and found that the utility appeared to succeed, but iohub info did not recognize the resulting .zarr.

I suspect this is because this example increases the number of rows? Does the utility accidentally assume the same number of rows and columns before and after?

I will send the files before and after via a separate channel.

Edit: you may need to delete the row named 0?

@talonchandler talonchandler dismissed their stale review September 19, 2024 18:06

stale, I made the changes

@talonchandler
Copy link
Contributor

@ieivanov @ziw-liu this is ready for review and testing.

The latest version renames wells in place without copying, updating the metadata appropriately. It adds new rows/columns and deletes empty rows/columns from the metadata as expected.

In addition to automated tests, I made a personal copy of Ivan's /hpc/projects/intracellular_dashboard/viral-sensor/2024_06_13_SEC61_TOMM20_ZIKV_DENGUE_1/0-convert/2024_06_13_SEC61_TOMM20_ZIKV_DENGUE.zarr/ and tested it with iohub rename-wells -i plate.zarr -c names.csv where names.csv is a headerless two-column file:

0/1,A/1
0/2,A/2
0/3,A/3
0/4,A/4
0/5,B/4
0/6,B/3
0/7,B/2
0/8,B/1

This test met my standards:

  • the renaming was very fast (no copying)
  • iohub info worked correctly before and after
  • no evidence of old well names in .zattrs
  • napari-iohub can read and display the new well names

@ieivanov
Copy link
Contributor

ieivanov commented Oct 1, 2024

I ran into a bad error trying to use a CSV file like this:

0/1, B/1

The extra white space cased problems. I've added changed to strip any white spaces and check for correct well names (using PlateAxisMeta) before making any changes to the zarr store. In the process I also refactored rename_wells into its own file, outside cli.py.

@ieivanov
Copy link
Contributor

ieivanov commented Oct 1, 2024

Tests fails because of #253. The tests to rename well pass successfully

@talonchandler
Copy link
Contributor

Nice bug find and fix!

I like it in its own file, but do you want it in iohub/cli/rename_wells.py instead of iohub/rename_wells.py? I'm flexible.

Ready to merge from my perspective.

@ieivanov
Copy link
Contributor

ieivanov commented Oct 1, 2024

I like it in its own file, but do you want it in iohub/cli/rename_wells.py instead of iohub/rename_wells.py? I'm flexible.

The convert and print_into functions are also at the iohub/ level. I think it's fine for now, we could organize the files a bit later.

Similar to set_scale, should we write metadata on the prior well name?

@ieivanov ieivanov merged commit 866db46 into czbiohub-sf:main Oct 3, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request NGFF OME-NGFF (OME-Zarr format)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants