Ngio projection - Githubissues

lorenzocerrone commented 1 week ago

Add ngio to dependecies
Refactor projection task to use ngio

Checklist before merging

[x] I added an appropriate entry to CHANGELOG.md

github-actions[bot] commented 1 week ago

Coverage report

Click to see where and how coverage changed

File	Statements	Missing	Coverage	Coverage (new stmts)	Lines missing
fractal_tasks_core/tables
v1.py	$This PR doesn't change the number of statements in fractal_tasks_core/tables/v1.py, which is 122.$	$This PR adds 11 to the number of statements missing coverage in fractal_tasks_core/tables/v1.py, taking it from 5 to 16.$	$This PR removes 9.77 percentage points from the coverage rate in fractal_tasks_core/tables/v1.py, taking it from 92.52% (117/122) to 82.75% (106/122).$	$This PR does not seem to add statements to fractal_tasks_core/tables/v1.py.$
fractal_tasks_core/tasks
projection.py	$This PR removes 10 from the number of statements in fractal_tasks_core/tasks/projection.py, taking it from 66 to 56.$	$This PR removes 2 from the number of statements missing coverage in fractal_tasks_core/tasks/projection.py, taking it from 6 to 4.$	$This PR adds 0.20 percentage points to the coverage rate in fractal_tasks_core/tasks/projection.py, taking it from 89.18% (60/66) to 89.39% (52/56).$	$In this PR, 43 new statements are added to fractal_tasks_core/tasks/projection.py, 41 of which are covered (95.34%).$	29, 77
Project Total

This report was generated by python-coverage-comment-action

jluethi commented 1 week ago

This is great @lorenzocerrone !

A few brief questions: Have you tried running this projection task on a zyx dataset (no channel or time dimension)? ~Could we easily add a synthetic test case for that using an ngio generated tiny zyx OME-Zarr. That then closes https://github.com/fractal-analytics-platform/fractal-tasks-core/issues/840~

Also, would we expect this task to already work with time-data, e.g. an tczyx dataset?

How much we should test in tasks-core? We test separately in tasks core only when there is task logic specifically to those cases

Can we add an option to copy over additional tables (e.g. user can deactivate that, but we copy non-roi tables by default). We'd keep them & ignore edge-cases (like missing label images) for now. TODO Joel: provide example dataset of condition tables for this. Approach: First copy them as they are (by adding a include_table flag to the derive function or by adding a copy_tables function). Then modify the ROI tables in place in the new Zarr => let's go with a copy_tables function

TODO for myself:

[ ] Check how we need to provide the metadata update for https://github.com/fractal-analytics-platform/fractal-tasks-core/issues/841. Likely a change to the image_list_update_dict
[ ] Run example 01 with this branch, to check for https://github.com/fractal-analytics-platform/fractal-tasks-core/issues/823

tcompa commented 1 week ago

Thanks @lorenzocerrone! I added a few small comments, but the pattern is often the same (I am no expert in ngio, and then questions pop up frequently independently on the actual PR). I think we are at a stage where it would be useful to briefly discuss this live.

tcompa commented 15 hours ago

I confirm that setting both origin and attributes/plate in the output is valid, and the value which will take priority is the one in attributes/plate.

fractal-analytics-platform / fractal-tasks-core

Ngio projection #866

Checklist before merging

Coverage report