Set ROI origin to match well origin #524

tcompa · 2023-09-18T08:13:13Z

Shift the FOV/well ROI tables so that their minimum XYZ values are at 0.
Remove workaround ROI-table origin #339 (comment)
Adapt zenodo_zarr fixture to "fix" current zenodo datasets, and open issue about update (ref Update new datasets to Zenodo #526)
Fix bug in creation of bounding-box ROIs #460
Review reset_origin=True implementation in convert_ROI_table_to_indices #507
reset_origin should not modify its input #525
Do not use mutable default in prepare_FOV_ROI_table and prepare_well_ROI_table #527

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

Checklist before merging

I added an appropriate entry to CHANGELOG.md

…_table` (ref #339)

github-actions · 2023-09-18T08:23:33Z

Coverage report

The coverage rate went from 89.84% to 89.8% ⬇️
The branch rate is 83%.

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

…eset_origin`

… (ref #460)

tcompa · 2023-09-18T11:38:30Z

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 #339 (comment) 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:

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).
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 · 2023-09-18T13:56:00Z

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 · 2023-09-19T06:50:30Z

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 f15863d.

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 · 2023-09-19T08:00:12Z

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 :)

fractal_tasks_core/lib_regions_of_interest.py

jluethi · 2023-09-19T15:29:35Z

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 · 2023-09-19T15:41:16Z

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 · 2023-09-20T06:56:17Z

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 · 2023-09-20T07:02:13Z

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 · 2023-09-20T07:10:03Z

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 · 2023-09-20T07:10:08Z

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

tcompa added 2 commits September 18, 2023 10:07

Add _reset_origin_in_dataframe helper function

b046954

Include origin-reset in prepare_FOV_ROI_table and `prepare_well_ROI…

26f38a4

…_table` (ref #339)

tcompa linked an issue Sep 18, 2023 that may be closed by this pull request

ROI-table origin #339

Closed

tcompa added 2 commits September 18, 2023 10:18

Remove reset_origin argument from convert_ROI_table_to_indices

42c14fa

Also remove reset_origin argument from a test

83c29e9

tcompa added 4 commits September 18, 2023 10:55

Update zenodo_zarr fixture in view of #339

f2bb44f

Add type hints

e8b246f

Add failing test for #525

e3af57b

Do not modify input of reset_origin (close #525)

97d3311

tcompa mentioned this pull request Sep 18, 2023

Update new datasets to Zenodo #526

Closed

2 tasks

tcompa added 3 commits September 18, 2023 11:11

Update comment in tests

00b9bf8

Add origin_zyx argument to array_to_bounding_box_table

fb3d6e4

Set origin_zyx in cellpose task (close #460)

7ec1d05

This was referenced Sep 18, 2023

Fix bug in creation of bounding-box ROIs #460

Closed

Review reset_origin=True implementation in convert_ROI_table_to_indices #507

Closed

tcompa added 4 commits September 18, 2023 12:06

Fix mutable argument (close #527) and improve docstrings

4a81d64

Add docstring

6f211a1

Remove redundant _reset_origin_in_dataframe function in favor of `r…

fcf8391

…eset_origin`

Update test_workflow_secondary_labeling to run at the per-FOV level…

06c4e12

… (ref #460)

tcompa marked this pull request as ready for review September 18, 2023 11:20

tcompa added 4 commits September 18, 2023 13:22

Update CHANGELOG

1f63d6f

Improve docstring

f6e9307

Improve docstring

ec608e4

Update CHANGELOG [skip ci]

744197f

tcompa requested a review from jluethi September 18, 2023 11:33

Remove reset_origin call from apply_registration_to_ROI_tables.py

f15863d

jluethi reviewed Sep 19, 2023

View reviewed changes

fractal_tasks_core/lib_regions_of_interest.py Show resolved Hide resolved

jluethi reviewed Sep 19, 2023

View reviewed changes

fractal_tasks_core/lib_regions_of_interest.py Show resolved Hide resolved

jluethi reviewed Sep 19, 2023

View reviewed changes

fractal_tasks_core/lib_regions_of_interest.py Show resolved Hide resolved

jluethi reviewed Sep 19, 2023

View reviewed changes

fractal_tasks_core/lib_regions_of_interest.py Show resolved Hide resolved

tcompa added 2 commits September 19, 2023 17:38

Improve docstrings

8544a99

Add return type hint

5f06e6e

tcompa added 2 commits September 19, 2023 17:41

Improve docstring

e2aebf7

Fix indentation in docstrings

1ee0a56

Update CHANGELOG

8ef4b91

tcompa added 2 commits September 20, 2023 09:05

Improve docstring

a97a7b8

Improve docstring

f7313a2

jluethi approved these changes Sep 20, 2023

View reviewed changes

tcompa merged commit e9b3996 into main Sep 20, 2023

tcompa deleted the 339-roi-table-origin branch September 20, 2023 07:15

This was referenced Sep 20, 2023

Add checks on ROI origin #530

Closed

Could we only include the tasks subpackage when fractal-tasks extra is installed? #531

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Set ROI origin to match well origin #524

Set ROI origin to match well origin #524

Uh oh!

tcompa commented Sep 18, 2023 •

edited

Loading

Uh oh!

github-actions bot commented Sep 18, 2023 •

edited

Loading

Uh oh!

tcompa commented Sep 18, 2023 •

edited

Loading

Uh oh!

jluethi commented Sep 18, 2023

Uh oh!

tcompa commented Sep 19, 2023

Uh oh!

jluethi commented Sep 19, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jluethi commented Sep 19, 2023

Uh oh!

jluethi commented Sep 19, 2023

Uh oh!

tcompa commented Sep 20, 2023

Uh oh!

jluethi commented Sep 20, 2023

Uh oh!

tcompa commented Sep 20, 2023

Uh oh!

jluethi commented Sep 20, 2023

Uh oh!

Uh oh!

Set ROI origin to match well origin #524

Set ROI origin to match well origin #524

Uh oh!

Conversation

tcompa commented Sep 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist before merging

Uh oh!

github-actions bot commented Sep 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage report

Uh oh!

tcompa commented Sep 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jluethi commented Sep 18, 2023

Uh oh!

tcompa commented Sep 19, 2023

Uh oh!

jluethi commented Sep 19, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jluethi commented Sep 19, 2023

Uh oh!

jluethi commented Sep 19, 2023

Uh oh!

tcompa commented Sep 20, 2023

Uh oh!

jluethi commented Sep 20, 2023

Uh oh!

tcompa commented Sep 20, 2023

Uh oh!

jluethi commented Sep 20, 2023

Uh oh!

Uh oh!

tcompa commented Sep 18, 2023 •

edited

Loading

github-actions bot commented Sep 18, 2023 •

edited

Loading

tcompa commented Sep 18, 2023 •

edited

Loading