-
Notifications
You must be signed in to change notification settings - Fork 13
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
Adding support for Zarr stitching/fusion #41
base: main
Are you sure you want to change the base?
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
zarr_processor.py
Outdated
self.tstore = tstore | ||
|
||
def __getitem__(self, ind): | ||
print(ind) |
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.
Please remove.
zarr_processor.py
Outdated
class ZarrFusion(warp.StitchAndRender3dTiles): | ||
""" | ||
Fusion renderer subclass | ||
that implements data loading for Zarr datasets. |
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.
Can probably wrap this into a single line with smth like "Fusion renderer loading tile data from Zarr."
zarr_utils.py
Outdated
class CloudStorage(Enum): | ||
""" | ||
Documented Cloud Storage Options | ||
""" |
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.
Please reduce short docstrings like this one into a single line for consistency with the rest of the code.
zarr_utils.py
Outdated
downsample_exp: int | ||
|
||
|
||
def open_zarr_gcs(bucket: str, path: str): |
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.
Please add a return type annotation.
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.
We're trying to keep the main directory of sofima focused on the main functionality provided by the module. Would you mind moving some files around with this, e.g.
zarr_utils.py -> io/zarr.py
zarr_processor.py -> utils/zarr_register_and_fuse_3d.py
zarr_utils.py
Outdated
}).result() | ||
|
||
|
||
def open_zarr_s3(bucket: str, path: str): |
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.
Please add a return type annotation.
zarr_utils.py
Outdated
|
||
|
||
def load_zarr_data(params: ZarrDataset | ||
) -> tuple[list[ts.TensorStore], tuple[int, int, int]]: |
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.
Consider using stitch_elastic.ShapeXYZ for tuple[int, int, int] which conveys a bit more semantic meaning.
zarr_utils.py
Outdated
@dataclass | ||
class ZarrDataset: | ||
""" | ||
Parameters for locating Zarr dataset living on the cloud. |
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.
Please add an Args: docstring explaining the different parameters (particularly tile_name and downsample_exp).
zarr_utils.py
Outdated
|
||
# Standardize size of tile volumes | ||
for i, tile_vol in enumerate(tile_volumes): | ||
tile_volumes[i] = tile_vol[:, :, :min_z, :min_y, :min_x] |
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.
Please mention this behavior (tiles getting cropped at origin to the smallest tile in the set) in the docstring.
zarr_utils.py
Outdated
|
||
|
||
def write_zarr(bucket: str, shape: list, path: str): | ||
""" |
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.
Please use a single sentence docstring followed by an explanation of the arguments in the Args: section.
stitch_elastic.py
Outdated
@@ -134,8 +134,8 @@ def compute_flow_map3d( | |||
curr_box = bounding_box.BoundingBox(start=(0, 0, 0), size=tile_shape) | |||
nbor_box = bounding_box.BoundingBox( | |||
start=( | |||
tile_shape[0] * (1 - axis) + offset[0], |
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.
Why this change?
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.
Could you please completely revert the changes to this file?
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.
Could you please completely revert the changes to this file?
Could you please also:
|
Hello, this PR adds zarr_processor.py, an entrypoint for processing Zarr datasets.
zarr_processor.py imports:
In addition, I tweaked fine_registration.py and processor.warp.py (deleting unused fields, renaming variables) for readability.