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 2D support for Cellpose & napari workflows task (closes #398) #403

Merged
merged 50 commits into from
Jun 13, 2023

Conversation

jluethi
Copy link
Collaborator

@jluethi jluethi commented Jun 7, 2023

Adding support to process 2D OME-Zarr files with the Cellpose & napari workflows task.

The resulting label images by cellpose are always 3D arrays. If the input image was 2 dimensional, it's of shape (1, shape_y, shape_x).

What still needs to be done:

  • Check whether any tests fail
  • Add tests for new lib function
  • Test napari workflows further: I covered some scenarios, but I'm not sure whether the "calculate a label image in napari workflows based on an actual 2D image OME-Zarr intensity image" would work as expected. I think it should (and should just be saved as an actual 2D image), but to be tested a bit further.
  • Plus test the different interactions with expected_dimensions and if there are weird edge cases there

I want to cover 1 & 2 for this PR. Point 3 may be moved to another issue => #411

@jluethi jluethi linked an issue Jun 7, 2023 that may be closed by this pull request
@github-actions
Copy link

github-actions bot commented Jun 8, 2023

Coverage report

The coverage rate went from 82.94% to 83.64% ⬆️
The branch rate is 77%.

94.02% of new lines are covered.

Diff Coverage details (click to unfold)

fractal_tasks_core/lib_regions_of_interest.py

100% of new lines are covered (82.41% of the complete file).

fractal_tasks_core/tasks/cellpose_segmentation.py

94.11% of new lines are covered (82.38% of the complete file).
Missing lines: 458

fractal_tasks_core/tasks/napari_workflows_wrapper.py

81.25% of new lines are covered (88.86% of the complete file).
Missing lines: 323, 339, 462

fractal_tasks_core/lib_zattrs_utils.py

100% of new lines are covered (94.93% of the complete file).

fractal_tasks_core/init.py

100% of new lines are covered (100% of the complete file).

@jluethi jluethi requested a review from tcompa June 9, 2023 09:51
@jluethi
Copy link
Collaborator Author

jluethi commented Jun 9, 2023

@tcompa This would PR be ready for a review if you have time for one. Some work is postponed to #411 to get the core functionality already in now.

@jluethi
Copy link
Collaborator Author

jluethi commented Jun 9, 2023

Now added a workaround to handle coordinateTransformations that follow the spec better than us, see #420 (comment)

@jluethi
Copy link
Collaborator Author

jluethi commented Jun 9, 2023

There was a second place where we currently have the assumption that coordinateTransformations always consist of z, y, x in the lib_zattrs_utils: The rescale_datasets. I now switch this such that it also works with czyx (or tczyx). But we should of course make this more general. This fix is just so that our tasks don't break with OME-Zarrs with czyx coordinateTransformations.

@tcompa
Copy link
Collaborator

tcompa commented Jun 13, 2023

Test napari workflows further: I covered some scenarios, but I'm not sure whether the "calculate a label image in napari workflows based on an actual 2D image OME-Zarr intensity image" would work as expected. I think it should (and should just be saved as an actual 2D image), but to be tested a bit further.

It does work, see #398 (comment)

@tcompa
Copy link
Collaborator

tcompa commented Jun 13, 2023

Plus test the different interactions with expected_dimensions and if there are weird edge cases there

If you run a napari_workflows_wrapper labeling task with expected_dimension=3 and a CYX input, you will get

ValueError('Something wrong: label_shape=(540, 1280) but expected_dimensions=3')

(as verified with the new test_napari_workflow_CYX_wrong_dimensions test).

Are there other useful checks that come to mind?

@jluethi
Copy link
Collaborator Author

jluethi commented Jun 13, 2023

Are there other useful checks that come to mind?

Should we include a test that a 3D input with the z dimension > 1 fails when we set expected_dimension=2?

Let's not go too deep here though. I think when doing the axes refactor, we may want to reconsider whether expected_dimensions is a parameter that really needs to be exposed to the user

@tcompa
Copy link
Collaborator

tcompa commented Jun 13, 2023

Should we include a test that a 3D input with the z dimension > 1 fails when we set expected_dimension=2?

We already had the following

cases = [
    (2, 2, True),
    (2, 3, False),
    (3, 3, True),
    (3, 2, True),
]


@pytest.mark.parametrize(
    "expected_dimensions,zarr_dimensions,expected_success", cases
)
def test_expected_dimensions(
    expected_dimensions: int,
    zarr_dimensions: int,
    expected_success: bool,
    tmp_path: Path,
    testdata_path: Path,
    zenodo_zarr: List[str],
    zenodo_zarr_metadata: List[Dict[str, Any]],
):

    # Prepare zarr
    zarr_path = tmp_path / "tmp_out/"
    if zarr_dimensions == 2:
        metadata = prepare_2D_zarr(
            str(zarr_path),
            zenodo_zarr,
            zenodo_zarr_metadata,
            remove_labels=True,
        )
    else:
        metadata = prepare_3D_zarr(
            str(zarr_path), zenodo_zarr, zenodo_zarr_metadata
        )
    debug(zarr_path)
    debug(metadata)

    # First napari-workflows task (labeling)
    workflow_file = str(
        testdata_path / "napari_workflows/wf_5-labeling_only.yaml"
    )
    input_specs: Dict[str, Dict[str, Union[str, int]]] = {
        "input_image": {"type": "image", "wavelength_id": "A01_C01"},
    }
    output_specs: Dict[str, Dict[str, Union[str, int]]] = {
        "output_label": {
            "type": "label",
            "label_name": "label_DAPI",
        },
    }

    for component in metadata["image"]:
        arguments = dict(
            input_paths=[str(zarr_path)],
            output_path=str(zarr_path),
            metadata=metadata,
            component=component,
            input_specs=input_specs,
            output_specs=output_specs,
            workflow_file=workflow_file,
            input_ROI_table="FOV_ROI_table",
            level=3,
            expected_dimensions=expected_dimensions,
        )
        if expected_success:
            napari_workflows_wrapper(**arguments)
        else:
            with pytest.raises(ValueError):
                napari_workflows_wrapper(**arguments)

which I can tweak to also include the CYX case.

@tcompa
Copy link
Collaborator

tcompa commented Jun 13, 2023

which I can tweak to also include the CYX case.

Done now (461c5de), with this list of cases

zarr_dimensions axes expected_dimensions expected_success
2 CYX 2 success
2 CYX 3 fail
2 CZYX 2 success
2 CZYX 3 success
3 CZYX 2 fail
3 CZYX 3 success

@tcompa tcompa marked this pull request as ready for review June 13, 2023 11:57
@tcompa tcompa merged commit 7097648 into main Jun 13, 2023
@tcompa tcompa deleted the 398_2D_support branch June 13, 2023 13:32
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.

Support 2D-only OME-Zarrs?
2 participants