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 overwrite argument to cellpose task #458

Closed
Tracked by #491
tcompa opened this issue Jul 17, 2023 · 6 comments · Fixed by #499
Closed
Tracked by #491

Add overwrite argument to cellpose task #458

tcompa opened this issue Jul 17, 2023 · 6 comments · Fixed by #499

Comments

@tcompa
Copy link
Collaborator

tcompa commented Jul 17, 2023

This is part of a broader discussion, and not necessarily restricted to cellpose task. Together with issues like #351 or #279 (and to related updates in fractal-server), this issue would contribute to enabling the "I want to play around on a subset of data as part of an experimental workflow branch" use case.

@jluethi
Copy link
Collaborator

jluethi commented Jul 17, 2023

Could come with an overwrite=True option that is even on by default.

@tcompa
Copy link
Collaborator Author

tcompa commented Aug 28, 2023

Default: True

Notes:

  1. overwrite will define the behavior for two cases at the same time:
    • Writing labels
    • Writing tables (in case the output-ROI optional parameter is set)
  2. We should move the check on existing ROIs to the beginning of the task

@tcompa tcompa changed the title Make cellpose task overwrite existing labels Add overwrite argument to cellpose task Aug 29, 2023
@tcompa
Copy link
Collaborator Author

tcompa commented Aug 30, 2023

As part of #500, we introduced the write_table helper function. In a very similar way, in c6dcae3 I introduced a prepare_label_group helper function, which shares a lot of logic with write_table (e.g. the overwrite-related checks and the setting of attributes for the new label group). The main difference is that prepare_label_group does not handle label writing (while write_table does, through anndata), see explanation in the docstring below.

Screenshot from 2023-08-30 09-22-25

Any feedback is welcome, both on whether we should have this helper function at all and about how it can improve. Note that we could obviously abstract some of the logic shared by write_table and prepare_image_group, but this will make no difference for someone writing a task and therefore does not represent a priority IMO.

@tcompa
Copy link
Collaborator Author

tcompa commented Aug 30, 2023

As of d9bc795, we are now also using the write_table helper function as part of the cellpose task.

@tcompa
Copy link
Collaborator Author

tcompa commented Aug 30, 2023

Note: IMO, the two diffs below (from create-ome-zarr and cellpose tasks) clearly supports the choice of introducing write_table

Screenshot from 2023-08-30 09-48-52

Screenshot from 2023-08-30 09-47-42

@jluethi
Copy link
Collaborator

jluethi commented Aug 30, 2023

Wow, that's a great improvement with write_tables indeed! It's both shorter and, importantly, also much simpler to understand what's happening!

I like the abstraction of the prepare_label_group for those same reasons. Was at first thinking about broadening it more, with also writing the tables. But as you state in the docstring, our region-based writing needs to happen in the task. So I'd say this is the correct trade-off. And looking at the diff for the cellpose task, this abstraction seems very worthwhile!

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 a pull request may close this issue.

2 participants