-
Notifications
You must be signed in to change notification settings - Fork 0
Plan wrapper functions to allow for easier cleanup of DAE setup changes #267
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
base: main
Are you sure you want to change the base?
Conversation
This is not currently achieved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quick non-functional review - please ask if any of my comments don't make sense. Will do a more thorough functional review when these initial API comments have been addressed.
| ['dae_table_wrapper](ibex_bluesky_core.plan_stubs.dae_table_wrapper) | ||
| ['num_periods_wrapper](ibex_bluesky_core.plan_stubs.num_periods_wrapper) | ||
| ['time_channels_wrapper](ibex_bluesky_core.plan_stubs.time_channels_wrapper) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just as an fyi, using the py:obj syntax renders slightly nicer. I also think it would be good to give a short description of that each one does here, rather than just dumping them as an unformatted list.
| @@ -0,0 +1,39 @@ | |||
| # Plan Wrappers | |||
|
|
|||
| Plan wrappers that temporarily modify DAE (Data Acquisition Electronics) settings during a plan, automatically restoring the original values afterwards. This ensures that your experiments don't permanently change instrument configuration. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Link to DAE pages?
| RE( | ||
| with_num_periods( | ||
| example_plan(), | ||
| dae=dae, | ||
| ) | ||
| ) | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the example we provide should be at the plan level, not a direct RE call.
| with_num_periods( | ||
| example_plan(), | ||
| dae=dae, | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm since with_num_periods doesn't actually change num_periods here, maybe it should be restore_num_periods instead.
The alternative is for the API to be:
with_num_periods(some_plan(), dae=dae, num_periods=10)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Tom, are you suggesting that we make another wrapper function?
restore_num_periods and with_num_periods?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I think we should only have one...
But it should either be:
with_num_periods(some_plan(), dae=dae, num_periods=10)
ORrestore_num_periods(some_plan(), dae=dae)
The difference between these two is basically API; in the bottom one someone has to manually call yield from bps.mv(dae.number_periods) themselves, in the top one it's just one call.
|
|
||
| UNUSED = 0 | ||
| DAQ = 1 | ||
| DWEll = 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it really DWEll capitalised like that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-> _dae_table_wrapper.py
same for the others
| "motor_scan", | ||
| "polling_plan", | ||
| "scan", | ||
| "with_num_periods", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a plan stub, not a plan, so shouldn't be re-exported here, but in plan_stubs/__init__.py instead.
| RE( | ||
| with_num_periods( | ||
| _dummy_plan_which_sets_periods(dae), | ||
| dae=dae, # type: ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this fail type-checking?
| result = await dae.number_of_periods.read() | ||
| assert result["DAE-number_of_periods-signal"]["value"] == original_settings |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think .get_value() may be cleaner here.
| assert after.wiring_filepath.endswith(expected_wiring) # type: ignore | ||
| assert after.spectra_filepath.endswith(expected_spectra) # type: ignore | ||
| assert after.detector_filepath.endswith(expected_detector) # type: ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why were these type-ignores needed?
Description of work
Three wrapper functions have been written to simplie DAE clean up:
with_num_periods(n)
with_time_channels(tcbs)
with_dae_tables(wiring, spectra, detector)
testing and documentation has also been updated for the functions.
Ticket
Link to Ticket
Acceptance criteria