-
Notifications
You must be signed in to change notification settings - Fork 7
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
Conversation
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: |
There was a problem hiding this 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.
rename-wells
CLI utility
There was a problem hiding this 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
?
@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
This test met my standards:
|
I ran into a bad error trying to use a CSV file like this:
The extra white space cased problems. I've added changed to strip any white spaces and check for correct well names (using |
Tests fails because of #253. The tests to rename well pass successfully |
Nice bug find and fix! I like it in its own file, but do you want it in Ready to merge from my perspective. |
The Similar to |
This PR adds:
rename-wells
utility with signatureiohub rename-wells -i input.zarr -c well-mapping.csv
, which renames the input well names in place. Thewell-mapping.csv
file has two columns with old and new names, and I have an examplewell-mapping.csv
in/iohub/docs/examples
ngff.rename_well
. The CLI uses this API.