pymmcore-plus / useq-schema

An implementation agnostic schema for describing a sequence of events during a multi-dimensional imaging acquisition.
https://pymmcore-plus.github.io/useq-schema/
BSD 3-Clause "New" or "Revised" License
15 stars 5 forks source link

fix: fix inherit sequence autofocus + add tests #98

Closed fdrgsp closed 1 year ago

fdrgsp commented 1 year ago

I found an autofocus_plan bug in the MDASequence iter_sequence method and this PR is a possible fix (also related to #97 ).

If we have a MDASequence with a position sub-sequence (like the one below) where the autofocus_plan is only specified in the main sequence, the autofocus_plan should be also inherited by the position sub-sequence. However at the moment this does not happen.

mda = MDASequence(
    channels=["DAPI", "FITC"],
    stage_positions=[(1, 2, 3),{"z": 10, "sequence":{"z_plan": {"range": 2, "step": 1}}}],
    autofocus_plan={"autofocus_device_name": "Z", "autofocus_motor_offset": 40, "axes": ("z",)}
)

by printing index and action type

for e in mda:
    print(e.index, e.action.type

we get

{'p': 0, 'c': 0} acquire_image
{'p': 0, 'c': 1} acquire_image
{'p': 1, 'c': 0, 'z': 0} acquire_image
{'p': 1, 'c': 0, 'z': 1} acquire_image
{'p': 1, 'c': 0, 'z': 2} acquire_image
{'p': 1, 'c': 1, 'z': 0} acquire_image
{'p': 1, 'c': 1, 'z': 1} acquire_image
{'p': 1, 'c': 1, 'z': 2} acquire_image

indicating that the auto shutter is never used. While we should instead autofocus performed at every z planes like so

{'p': 0, 'c': 0} acquire_image
{'p': 0, 'c': 1} acquire_image
{'p': 1, 'c': 0, 'z': 0} hardware_autofocus
{'p': 1, 'c': 0, 'z': 0} acquire_image
{'p': 1, 'c': 0, 'z': 1} hardware_autofocus
{'p': 1, 'c': 0, 'z': 1} acquire_image
{'p': 1, 'c': 0, 'z': 2} hardware_autofocus
{'p': 1, 'c': 0, 'z': 2} acquire_image
{'p': 1, 'c': 1, 'z': 0} hardware_autofocus
{'p': 1, 'c': 1, 'z': 0} acquire_image
{'p': 1, 'c': 1, 'z': 1} hardware_autofocus
{'p': 1, 'c': 1, 'z': 1} acquire_image
{'p': 1, 'c': 1, 'z': 2} hardware_autofocus
{'p': 1, 'c': 1, 'z': 2} acquire_image
codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (5652f37) 96.40% compared to head (49324cc) 96.40%.

:exclamation: Current head 49324cc differs from pull request most recent head e3a0625. Consider uploading reports for the commit e3a0625 to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #98 +/- ## ======================================= Coverage 96.40% 96.40% ======================================= Files 12 12 Lines 724 724 ======================================= Hits 698 698 Misses 26 26 ``` | [Impacted Files](https://app.codecov.io/gh/pymmcore-plus/useq-schema/pull/98?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pymmcore-plus) | Coverage Δ | | |---|---|---| | [src/useq/\_mda\_sequence.py](https://app.codecov.io/gh/pymmcore-plus/useq-schema/pull/98?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pymmcore-plus#diff-c3JjL3VzZXEvX21kYV9zZXF1ZW5jZS5weQ==) | `98.35% <100.00%> (ø)` | |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

tlambert03 commented 1 year ago

can you try to do this with

                sub_seq = position.sequence
                # if the sub-sequence doe not have an autofocus plan, we override it
                # with the parent sequence's autofocus plan
                if not sub_seq.autofocus_plan:
                    sub_seq = sub_seq.replace(autofocus_plan=sequence.autofocus_plan)

                # recurse into the sub-sequence
                yield from iter_sequence(
                    sub_seq,
                    ...

shouldn't that do the same thing?

tlambert03 commented 1 year ago

can you try to do this with

                sub_seq = position.sequence
                # if the sub-sequence doe not have an autofocus plan, we override it
                # with the parent sequence's autofocus plan
                if not sub_seq.autofocus_plan:
                    sub_seq = sub_seq.copy(update={'autofocus_plan':sequence.autofocus_plan})

                # recurse into the sub-sequence
                yield from iter_sequence(
                    sub_seq,
                    ...

shouldn't that do the same thing?

yeah works... but you need to use copy() not replace ... so as to maintain the internal state of the should_autofocus function. see https://github.com/fdrgsp/useq-schema/pull/4

fdrgsp commented 1 year ago

yeah works... but you need to use copy() not replace ... so as to maintain the internal state of the should_autofocus function. see fdrgsp#4

Yesssss!Works well👍🏻