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

set-scale utility #228

Merged

Conversation

aaronalvarezcz
Copy link
Contributor

@aaronalvarezcz aaronalvarezcz commented Jul 2, 2024

2024-09-17 update from @talonchandler.

Problem: Scale metadata in existing .zarr stores is frequently incorrect.

Solution: A small CLI utility set-scale that can fix incorrect metadata in a single CLI call.

Previous work: the shrimPy repository included an earlier version of this utility..

Interface:

iohub set-scale -i input.zarr/ -z 2.0 -y 0.325 -x 0.325

will result in

Updating all.zarr/A/1/0 z scale from 2.3 to 2.0.
Updating all.zarr/A/1/0 y scale from 2.6 to 0.325.
Updating all.zarr/A/1/0 x scale from 2.6 to 0.325.
Updating all.zarr/A/1/1 z scale from 2.3 to 2.0.
...
...
...
Updating all.zarr/D/4/8 x scale from 2.6 to 0.325.

Other notes:

  • I (@talonchandler) needed to move several parsing utilities into iohub. These utilities are widely used across compmicro's pipelines, so I think they deserve a place here.
  • -i input.zarr will expand a plate into a list of positions, then update the scale metadata of each position. -i input.zarr/*/*/* is equivalent.
  • this utility now accepts any subset of -t, -z, -y, and -x flags

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'll suggest:

  • reviving the -i option
  • trialing a full CLI and/or config interface instead of the current interactive
  • documenting and testing

iohub/cli/cli.py Outdated Show resolved Hide resolved
@ziw-liu ziw-liu added the enhancement New feature or request label Jul 2, 2024
@ziw-liu ziw-liu added this to the 0.2.0 milestone Jul 2, 2024
@ziw-liu
Copy link
Collaborator

ziw-liu commented Jul 2, 2024

Please add a description of the feature you are implementing and the problem it solves.

@ziw-liu ziw-liu added the NGFF OME-NGFF (OME-Zarr format) label Jul 3, 2024
iohub/cli/cli.py Outdated Show resolved Hide resolved
@talonchandler
Copy link
Contributor

After discussion, here's a list of TODOs before we're ready for wider review:

  • remove the dependency on mantis and copy over the -i option instead.
  • move the test from shrimpy over to iohub
  • make sure that tests are passing

@talonchandler talonchandler marked this pull request as draft September 17, 2024 20:25
@talonchandler talonchandler changed the title add update-scale-metadata util update-scale-metadata utility Sep 17, 2024
tests/cli/test_cli.py Outdated Show resolved Hide resolved
@talonchandler talonchandler changed the title update-scale-metadata utility set-scale utility Sep 19, 2024
@ieivanov
Copy link
Contributor

It's great that the OptionEatAll is now moving it iohub. It'll be good to add tests for it, and also test that -i input.zarr gets expanded into positions.

@talonchandler
Copy link
Contributor

@ieivanov I've added tests for OptionEatAll and plate-expansion behavior. I think this is ready for a final look.

@ieivanov
Copy link
Contributor

I've run out of time debugging, but I managed to fix an issue with updating the scale transform when the transform did not exist in the first place (datasets are created with identity transform by default).

I'm still seeing an issue that in the .zattrs file only "old_x": 1.0 is written even when updating all of X, Y, and Z.

Comment on lines 1066 to 1072
# Append old scale to metadata
if "iohub" not in self.zattrs:
iohub_dict = {}
else:
iohub_dict = self.zattrs["iohub"]
iohub_dict.update({f"old_{axis_name}": self.scale[axis_index]})
self.zattrs["iohub"] = iohub_dict
Copy link
Contributor

Choose a reason for hiding this comment

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

@ziw-liu our original line didn't update the metadata in the way we intended. My fault for not testing this.

@ziw-liu this is the simplest solution I could get to work, but it's a bit ugly. Should .zattrs["iohub"] be part of the pydantic models so that we can use dump_meta?

Copy link
Contributor

@ieivanov ieivanov left a comment

Choose a reason for hiding this comment

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

Let's merge!

@talonchandler talonchandler merged commit 5e223ec into czbiohub-sf:main Sep 26, 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.

5 participants