search

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
14 stars 6 forks source link

Set ROI origin to match well origin #524

Closed tcompa closed 11 months ago

tcompa commented 11 months ago

Close #339 Close #460 Close #525 Close #507 Close #527

Checklist before merging

github-actions[bot] commented 11 months ago

Coverage report

The coverage rate went from 89.84% to 89.8% :arrow_down: The branch rate is 83%.

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

tcompa commented 11 months ago

This is mostly ready on my side, and it should streamline the understanding of ROIs from now on. FOV/well ROIs will have zero origin, and we don't need the workaround https://github.com/fractal-analytics-platform/fractal-tasks-core/issues/339#issuecomment-1460020767 any more.

I also fixed some slightly unrelated ROI issues (notably the bounding-box ROIs when running cellpose over FOVs), see full list in the first post above.

@jluethi a review would be useful.

I don't have a full understanding of what parts of registration tasks are tested, and then I am not sure of whether there remain open issues related to the ROI origins. Some notes:

  1. I removed the reset_origin argument of convert_ROI_table_to_indices, and then I also removed reset_origin=False from apply_registration_to_image.py (see PR diff).
  2. I did not modify apply_registration_to_ROI_tables.py, but I'm now wondering whether the use of reset_origin(..) in this task is still relevant or not.
jluethi commented 11 months ago

Great, will review!

I did not modify apply_registration_to_ROI_tables.py, but I'm now wondering whether the use of reset_origin(..) in this task is still relevant or not.

If all tables already come with a reset origin, then this is not necessary anymore. We only needed to run reset_origins because FOV & well ROIs had non-0 origins.

tcompa commented 11 months ago

If all tables already come with a reset origin, then this is not necessary anymore. We only needed to run reset_origins because FOV & well ROIs had non-0 origins.

This makes sense, thanks. I just removed it with https://github.com/fractal-analytics-platform/fractal-tasks-core/pull/524/commits/f15863d2bc6651a97fdccccaf304348cfa81695b.

test_registration.py::test_multiplexing_registration is passing, and uses apply_registration_to_ROI_tables. Is this enough as a check, or do we need new tests?

jluethi commented 11 months ago

Hmm, good question. I think the tests would catch it if it calculates the registration wrong (because I have some checks on the amount of 0-padded pixels and such). But I'll run it on example 03 as part of the review and verify the results visually as well, just to be sure :)

jluethi commented 11 months ago

The changes look good to me. Great that you updated docstrings @tcompa ! I added a few minor points to where we could clarify them further, but that's about it.

I'll also run some tests on example 03, will report results :)

jluethi commented 11 months ago

In local tests, I keep running into this error (both for example 01 and example 03):

Traceback (most recent call last):
  File "/Users/joel/Library/CloudStorage/Dropbox/Joel/BioVisionCenter/Code/fractal/fractal-demos/examples/server/FRACTAL_TASKS_DIR/.fractal/fractal_tasks_core0.11.0/venv/lib/python3.9/site-packages/fractal_tasks_core/tasks/yokogawa_to_ome_zarr.py", line 260, in <module>
    run_fractal_task(
  File "/Users/joel/Library/CloudStorage/Dropbox/Joel/BioVisionCenter/Code/fractal/fractal-demos/examples/server/FRACTAL_TASKS_DIR/.fractal/fractal_tasks_core0.11.0/venv/lib/python3.9/site-packages/fractal_tasks_core/tasks/_utils.py", line 79, in run_fractal_task
    metadata_update = task_function(**pars)
  File "pydantic/decorator.py", line 40, in pydantic.decorator.validate_arguments.validate.wrapper_function
  File "pydantic/decorator.py", line 134, in pydantic.decorator.ValidatedFunction.call
  File "pydantic/decorator.py", line 206, in pydantic.decorator.ValidatedFunction.execute
  File "/Users/joel/Library/CloudStorage/Dropbox/Joel/BioVisionCenter/Code/fractal/fractal-demos/examples/server/FRACTAL_TASKS_DIR/.fractal/fractal_tasks_core0.11.0/venv/lib/python3.9/site-packages/fractal_tasks_core/tasks/yokogawa_to_ome_zarr.py", line 173, in yokogawa_to_ome_zarr
    sample = imread(tmp_images.pop())
  File "/Users/joel/Library/CloudStorage/Dropbox/Joel/BioVisionCenter/Code/fractal/fractal-demos/examples/server/FRACTAL_TASKS_DIR/.fractal/fractal_tasks_core0.11.0/venv/lib/python3.9/site-packages/dask/array/image.py", line 49, in imread
    imread = imread or sk_imread
NameError: name 'sk_imread' is not defined

I do have Python 3.9.0 on that machine though. Given that our CI is fine, I'll have to look into updating python & retrying tomorrow

tcompa commented 11 months ago

In local tests, I keep running into this error (both for example 01 and example 03):

Traceback (most recent call last):
  File "/Users/joel/Library/CloudStorage/Dropbox/Joel/BioVisionCenter/Code/fractal/fractal-demos/examples/server/FRACTAL_TASKS_DIR/.fractal/fractal_tasks_core0.11.0/venv/lib/python3.9/site-packages/fractal_tasks_core/tasks/yokogawa_to_ome_zarr.py", line 260, in <module>
    run_fractal_task(
  File "/Users/joel/Library/CloudStorage/Dropbox/Joel/BioVisionCenter/Code/fractal/fractal-demos/examples/server/FRACTAL_TASKS_DIR/.fractal/fractal_tasks_core0.11.0/venv/lib/python3.9/site-packages/fractal_tasks_core/tasks/_utils.py", line 79, in run_fractal_task
    metadata_update = task_function(**pars)
  File "pydantic/decorator.py", line 40, in pydantic.decorator.validate_arguments.validate.wrapper_function
  File "pydantic/decorator.py", line 134, in pydantic.decorator.ValidatedFunction.call
  File "pydantic/decorator.py", line 206, in pydantic.decorator.ValidatedFunction.execute
  File "/Users/joel/Library/CloudStorage/Dropbox/Joel/BioVisionCenter/Code/fractal/fractal-demos/examples/server/FRACTAL_TASKS_DIR/.fractal/fractal_tasks_core0.11.0/venv/lib/python3.9/site-packages/fractal_tasks_core/tasks/yokogawa_to_ome_zarr.py", line 173, in yokogawa_to_ome_zarr
    sample = imread(tmp_images.pop())
  File "/Users/joel/Library/CloudStorage/Dropbox/Joel/BioVisionCenter/Code/fractal/fractal-demos/examples/server/FRACTAL_TASKS_DIR/.fractal/fractal_tasks_core0.11.0/venv/lib/python3.9/site-packages/dask/array/image.py", line 49, in imread
    imread = imread or sk_imread
NameError: name 'sk_imread' is not defined

I do have Python 3.9.0 on that machine though. Given that our CI is fine, I'll have to look into updating python & retrying tomorrow

The obvious comment is that you may be missing scikit-image in the fractal-tasks-core 0.11.0 venv, but why? Just to make sure: did you include the fractal-tasks package extra? Because that's where scikit-image is defined.

Other than that, we can debug it further - but I don't see any connection with the current PR.

jluethi commented 11 months ago

Just to make sure: did you include the fractal-tasks package extra? Because that's where scikit-image is defined.

Ah, that was it! 🤦🏻‍♂️

Downside of trying to do testing on a rush. Interesting though to see that this is how the issue pops up if someone forgets to include the extras!

Retrying now :)

tcompa commented 11 months ago

The changes look good to me. Great that you updated docstrings @tcompa ! I added a few minor points to where we could clarify them further, but that's about it.

Thanks for the review. I fixed the minor points and I don't have anything more to add.

I'll also run some tests on example 03, will report results :)

This would be the last check, and then I think we could merge this.

jluethi commented 11 months ago

Ok, I can now confirm that the multiplexing examples still work as expected. PR approved :)