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

Introduce/update overwrite parameter for most tasks #499

Merged
merged 76 commits into from
Sep 4, 2023

Conversation

tcompa
Copy link
Collaborator

@tcompa tcompa commented Aug 28, 2023

Close #485
Close #492
Close #493
Close #494
Close #495
Close #496
Close #497
Close #458
Close #500

Checklist before merging

  • I added an appropriate entry to CHANGELOG.md
  • Add tests for all (most?) cases. Note: there are basically three cases to test for each task: no existing output, fail due to existing output and overwrite=False, actually overwrite existing output. The first one is the one we are already testing, the second one should be somewhat easy (every time we run a task for the first time, we also re-run it right away with overwrite=False and catch the error).
  • Maybe remove some of the new tests to make the CI a bit faster? Fixed with Proof of principle napari workflows mock #504
  • Add unit tests for prepare_label_group (mostly similar to the ones for write_table)
  • Review manifest updates
  • Fix error message in MIP task
  • If/when we remove the contents of a zarr group through the zarr API, let's wrap this call in a small function that also logs what we are removing.
  • Rename lib_zarr, or move its functions to more appropriate modules.

@tcompa tcompa linked an issue Aug 28, 2023 that may be closed by this pull request
9 tasks
@github-actions
Copy link

github-actions bot commented Aug 28, 2023

Coverage report

The coverage rate went from 89.21% to 89.94% ⬆️
The branch rate is 84%.

88.95% of new lines are covered.

Diff Coverage details (click to unfold)

fractal_tasks_core/tasks/copy_ome_zarr.py

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

fractal_tasks_core/tasks/napari_workflows_wrapper.py

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

fractal_tasks_core/tasks/create_ome_zarr.py

60% of new lines are covered (82.9% of the complete file).
Missing lines: 199, 215, 269, 320

fractal_tasks_core/tasks/illumination_correction.py

60% of new lines are covered (81.2% of the complete file).
Missing lines: 61, 182, 222, 237

fractal_tasks_core/tasks/create_ome_zarr_multiplex.py

77.77% of new lines are covered (87.73% of the complete file).
Missing lines: 294, 337

fractal_tasks_core/tasks/cellpose_segmentation.py

81.81% of new lines are covered (84.53% of the complete file).
Missing lines: 387, 603

fractal_tasks_core/lib_write.py

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

fractal_tasks_core/lib_ROI_overlaps.py

0% of new lines are covered (95.02% of the complete file).
Missing lines: 240

fractal_tasks_core/tasks/maximum_intensity_projection.py

80% of new lines are covered (86.48% of the complete file).
Missing lines: 114, 133

fractal_tasks_core/tasks/yokogawa_to_ome_zarr.py

80% of new lines are covered (90.82% of the complete file).
Missing lines: 157, 208

fractal_tasks_core/lib_metadata_parsing.py

0% of new lines are covered (88.37% of the complete file).
Missing lines: 274, 370

@tcompa
Copy link
Collaborator Author

tcompa commented Aug 31, 2023

This PR is essentially ready on my side, and any feedback is welcome.
@jluethi could you go through it when you have time?

The PR is independent from the registration tasks introduced in #487, and the two branches have no conflict at the moment. I'd propose to merge this one first, and then address the overwrite-related features to the registration tasks somewhere else (either as part of #487 or as another PR about #498).

One last (minor) point to fix in this PR is maybe the code organization. Right now I added a broad-scope lib_zarr.py module, with these four functions:

open_zarr_group_with_overwrite
_write_elem_with_overwrite
write_table
prepare_label_group

I think this code organization is not ideal. Some possible improvements:

  1. Keep this module as is, and rename it to something more general (like lib_writing).
  2. Move ROI-related functions (i.e. the ones using anndata) into another module (lib_regions_of_interest.py, or even a new one).

@tcompa tcompa marked this pull request as ready for review August 31, 2023 08:33
@tcompa tcompa marked this pull request as draft August 31, 2023 08:33
@jluethi
Copy link
Collaborator

jluethi commented Aug 31, 2023

@tcompa That's awesome! I'll try to review as soon as possible. But may be Friday afternoon or Monday next week given the current conference

@tcompa
Copy link
Collaborator Author

tcompa commented Sep 1, 2023

@tcompa That's awesome! I'll try to review as soon as possible. But may be Friday afternoon or Monday next week given the current conference

No problem. The two branches at the moment have no conflicts (see registration_task...491-introduceupdate-overwrite-parameter-to-most-tasks), and they are both at a quite advanced stage - thus I think that extra care in the merging order is not needed.

Copy link
Collaborator

@jluethi jluethi left a comment

Choose a reason for hiding this comment

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

This PR looks great. I found one small issue in the illumination correction task and a few small docstring TBDs to be replaced.

Regarding the naming of lib_zarr: I agree, that's a bit too general. I'd be fine with lib_writing or any better ideas you have :) Could be lib_zarr_writing?

Other than that, ready to merge I'd say. Great work @tcompa ! 👏🏻

@tcompa
Copy link
Collaborator Author

tcompa commented Sep 4, 2023

Regarding the naming of lib_zarr: I agree, that's a bit too general. I'd be fine with lib_writing or any better ideas you have :) Could be lib_zarr_writing?

I went with lib_write, inspired by the (somewhat similar in scope) https://github.com/ome/ome-zarr-py/blob/master/ome_zarr/writer.py module.


In the future we may want to review the structure of the core library, and introduce some more consistent module naming and/or folder organization. We may also export a bunch of lib_* functions directly to the main namespace, so that they become available as from fractal_tasks_core import some_function (instead of fractal_tasks_core.lib_something import some_function).

@tcompa tcompa marked this pull request as ready for review September 4, 2023 06:29
@tcompa
Copy link
Collaborator Author

tcompa commented Sep 4, 2023

I found one small issue in the illumination correction task

  • The parameter name for zarr.create was indeed wrong.
  • I think the parameter of build_pyramid must be always set to True, in the current version.

Side comment: this task includes a few unreachable code paths which cannot be tested, we'll need to review everything once we support overwrite_input=False.

and a few small docstring TBDs to be replaced.

Fixed, thanks!

@tcompa
Copy link
Collaborator Author

tcompa commented Sep 4, 2023

This is ready to merge, as soon as we agree on the overwrite parameter in build_pyramid.

@jluethi
Copy link
Collaborator

jluethi commented Sep 4, 2023

Agreed on the build_pyramid overwrite, we can merge! :)

@tcompa tcompa merged commit 347a6df into main Sep 4, 2023
@tcompa tcompa deleted the 491-introduceupdate-overwrite-parameter-to-most-tasks branch September 4, 2023 10:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment