Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added number_of_frames instead of iterations #581

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ZohebShaikh
Copy link
Contributor

@ZohebShaikh ZohebShaikh commented Sep 17, 2024

closes #575

@ZohebShaikh ZohebShaikh linked an issue Sep 17, 2024 that may be closed by this pull request
@ZohebShaikh ZohebShaikh force-pushed the 575-more-flexible-multi-run-specification branch from d86e04c to c52e34b Compare September 17, 2024 11:41
@ZohebShaikh ZohebShaikh marked this pull request as ready for review September 17, 2024 13:35
@ZohebShaikh ZohebShaikh force-pushed the 575-more-flexible-multi-run-specification branch from e7c72ba to 31c7ded Compare September 18, 2024 15:35
Copy link
Collaborator

@coretl coretl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please can we have a test that looks like the tomography example?

src/ophyd_async/plan_stubs/_fly.py Outdated Show resolved Hide resolved
src/ophyd_async/plan_stubs/_fly.py Outdated Show resolved Hide resolved
src/ophyd_async/plan_stubs/_fly.py Outdated Show resolved Hide resolved
src/ophyd_async/plan_stubs/_fly.py Outdated Show resolved Hide resolved
]


async def test_hdf_panda_hardware_triggered_flyable_with_iterations(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have deleted test_hdf_panda_hardware_triggered_flyable_with_iterations and moved it to

@pytest.mark.parametrize("number_of_frames", [[1, 1, 1, 1], [2, 3, 100, 3]])
async def test_hardware_triggered_flyable(

As we are removing iterations

src/ophyd_async/core/_detector.py Outdated Show resolved Hide resolved
src/ophyd_async/core/_detector.py Outdated Show resolved Hide resolved
src/ophyd_async/core/_detector.py Outdated Show resolved Hide resolved
src/ophyd_async/core/_detector.py Outdated Show resolved Hide resolved
src/ophyd_async/core/_detector.py Outdated Show resolved Hide resolved
src/ophyd_async/core/_detector.py Outdated Show resolved Hide resolved
src/ophyd_async/core/_detector.py Outdated Show resolved Hide resolved
src/ophyd_async/core/_detector.py Outdated Show resolved Hide resolved
tests/core/test_flyer.py Outdated Show resolved Hide resolved
tests/core/test_flyer.py Show resolved Hide resolved
src/ophyd_async/core/_detector.py Outdated Show resolved Hide resolved
src/ophyd_async/core/_detector.py Outdated Show resolved Hide resolved
src/ophyd_async/core/_detector.py Outdated Show resolved Hide resolved
src/ophyd_async/core/_detector.py Outdated Show resolved Hide resolved
src/ophyd_async/core/_detector.py Outdated Show resolved Hide resolved
src/ophyd_async/core/_detector.py Outdated Show resolved Hide resolved
src/ophyd_async/core/_detector.py Outdated Show resolved Hide resolved
src/ophyd_async/core/_detector.py Outdated Show resolved Hide resolved
src/ophyd_async/core/_detector.py Outdated Show resolved Hide resolved
src/ophyd_async/core/_detector.py Outdated Show resolved Hide resolved
tests/core/test_flyer.py Show resolved Hide resolved
@ZohebShaikh ZohebShaikh force-pushed the 575-more-flexible-multi-run-specification branch from fb3343e to 3a56af0 Compare September 27, 2024 10:47
Copy link
Contributor

@evalott100 evalott100 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy with this, I think we should wait for a once over from @coretl before we merge.

Copy link
Contributor

@evalott100 evalott100 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed my mind, I'd quite like

#581 (comment)

Which means either:

1)

adding

                self._frames_completed += 1

to complete and removing

        self._frames_completed += self._frames_to_complete

from kickoff.

Or (and I actually think this is the nicer option)

2)

Changing its name to self._completable_frames and leaving a comment in init that it represents the total number of frames that will have been completed at the end of the next complete.

This is nicer because it means that

if self._completable_frames >= self._trigger_info.total_number_of_triggers:
            raise RuntimeError(

will still work - if we changed to increment by 1 then would probably still need this variable to make sure you can't kickoff too many times.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

More flexible multi-run specification
3 participants