-
Notifications
You must be signed in to change notification settings - Fork 2
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
Use dodal devices more widely for I24 #116
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #116 +/- ##
==========================================
+ Coverage 52.82% 54.10% +1.28%
==========================================
Files 23 26 +3
Lines 3311 3188 -123
==========================================
- Hits 1749 1725 -24
+ Misses 1562 1463 -99 ☔ View full report in Codecov by Sentry. |
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.
Thanks, couple of minor comments but take them or leave them
yield from bps.abs_set( | ||
beamstop.pos_select, BeamstopPositions.ROBOT, group=place | ||
) | ||
yield from bps.abs_set(backlight, BacklightPositions.OUT, group=place) | ||
yield from bps.mv(det_stage.z, 1300) | ||
yield from bps.wait(group=place) |
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.
Nit: mixing abs_set
with group
and mv
like this seems a bit odd it would be a bit better to do one of:
yield from bps.mv(
beamstop.pos_select, BeamstopPositions.ROBOT,
backlight, BacklightPositions.OUT,
det_stage.z, 1300
)
or
yield from bps.abs_set(
beamstop.pos_select, BeamstopPositions.ROBOT, group=place
)
yield from bps.abs_set(backlight, BacklightPositions.OUT, group=place)
yield from bps.abs_set(det_stage.z, 1300, group=place)
yield from bps.wait(group=place)
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'd go with the second one because for the backlight I've overloaded set as it needs to turn it on and off depending on position. Not sure move would achieve that in this case...
yield from bps.abs_set( | ||
beamstop.pos_select, BeamstopPositions.DATA_COLLECTION, group=place | ||
) | ||
yield from bps.abs_set(backlight, BacklightPositions.IN, group=place) | ||
yield from bps.wait(group=place) |
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.
Nit: As above, a mv
might be nicer?
@@ -665,12 +668,14 @@ def run_fixed_target_plan( | |||
tot_num_imgs = datasetsizei24( | |||
parameters.num_exposures, parameters.chip_type, parameters.map_type | |||
) | |||
wavelength = yield from bps.rd(dcm.wavelength_in_a) | |||
if parameters.detector_name == "eiger": | |||
logger.debug("Start nexus writing service.") | |||
call_nexgen( |
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.
Could: It would be nice to have a test that covered that call_nexgen
has been called with the expected params
... plus various tidy ups
Closes #115
(Edit: forgot to add, needs Dodal652)