Comply with table specs v1

fractal-analytics-platform / fractal-tasks-core

Main tasks for the Fractal analytics platform

https://fractal-analytics-platform.github.io/fractal-tasks-core/

BSD 3-Clause "New" or "Revised" License

12 stars 5 forks source link

Comply with table specs v1 #613

Closed tcompa closed 7 months ago

tcompa commented 8 months ago

Refs:

Close #593 Close #594 Close #602 Close #616 Close #617

Checklist before merging

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

github-actions[bot] commented 7 months ago

Coverage report

The coverage rate went from 90.48% to 90.74% :arrow_up: The branch rate is 84%.

None of the new lines are part of the tested code. Therefore, there is no coverage data about them.

tcompa commented 7 months ago

Relevant part of CHANGELOG:

Breaking changes in tasks:
- Make NapariWorkflowsOutput.label_name attribute required, and use it to fill the region["path"] table attribute (#613).
Breaking changes in core library:
- Changes import of lib_write.write_table into lib_tables.write_table (#613).
- Drop logger argument from write_table helper function (#613).
- Change imports from lib_ROI_overlaps into imports from lib_regions_of_interest (#613).
Other internal changes:
- Comply with table specs V1, by writing all required Zarr attributes (#613).
- Refactor modules related to tables and ROIs (#613).
Documentation
- Clarify table specs V1 (#613).

tcompa commented 7 months ago

This is ready on my side. @jluethi could you also have a look?

Most of the PR only adds features (mainly by adding V1 table attributes when missing, or using V1 ones to replace legacy ones), but it is breaking in a couple of points both for the tasks and for the core library -- see previous comment.

The rest of the diff is largely related to refactoring of modules and updating tests.

jluethi commented 7 months ago

I'm starting to review this PR. One thing I noticed: The table .zattrs for our well_ROI_table & FOV_ROI_table don't have that "type": "roi_table", that the spec says they should have. Neither in the 3D Zarr nor in the 2D MIP Zarr.

tcompa commented 7 months ago

The table .zattrs for our well_ROI_table & FOV_ROI_table don't have that "type": "roi_table", that the spec says they should have. Neither in the 3D Zarr nor in the 2D MIP Zarr.

Thanks! This is now tested with https://github.com/fractal-analytics-platform/fractal-tasks-core/pull/613/commits/73e4a8719aca6479f1b5a77c997bc30fba3d2df1, and fixed with https://github.com/fractal-analytics-platform/fractal-tasks-core/pull/613/commits/1361c79117bbf1c092e54fecaadb299f1b79c2a0.

The same fix is now needed at least within import_ome_zarr.

tcompa commented 7 months ago

The same fix is now needed at least within import_ome_zarr.

And in create-ome-zarr-multiplex

tcompa commented 7 months ago

The same fix is now needed at least within import_ome_zarr.

And in create-ome-zarr-multiplex

Both fixed with https://github.com/fractal-analytics-platform/fractal-tasks-core/pull/613/commits/62ddae0849b8c73ed54ddefe68324bd34d3b5212.

jluethi commented 7 months ago

Scenarios for type: 1) No type is provided (neither optional new arg, nor in attrs) => Fail 2) Type is provided with new arg => set the type in attrs to that new type 3) No type is provided in new arg, but it's in attrs => Fine

jluethi commented 7 months ago

Let's also tackle this one here: https://github.com/fractal-analytics-platform/fractal-tasks-core/issues/619

jluethi commented 7 months ago

[x] _write_table_v1 should raise errors for masking ROI table or feature table if they don't follow the write attrs structure