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

bp.count() needs a time key word argument #997

Closed
prjemian opened this issue Feb 26, 2018 · 7 comments
Closed

bp.count() needs a time key word argument #997

prjemian opened this issue Feb 26, 2018 · 7 comments

Comments

@prjemian
Copy link
Contributor

When counting, it is common to specify the time (seconds) or time basis (monitor counts) to be used. The bp.count() plan does not have this option. It is expected that a count() should be able to specify for how long as an atomic operation, such as bp.count(time=0.2).

Expected Behavior

When time is given, it sets the detector counting time.

One convention in common use is that positive time is in seconds, negative time is in monitor (detector) counts which means the monitor detector must be declared in advance.

Current Behavior

Detector count time must be configured before calling bp.count().

Possible Solution

Support time as an optional keyword to bp.count().

This will likely involve standardizing how the different detector types implement the count time or counting against a reference signal.

  • SynSignal : bps.mv(det.exposure_time, time))
  • EpicsScaler : bps.mv(det.preset_time, time)
  • ScalerCH : bps.mv(det.preset_time, time)
  • areadetector.DetectorBase : bps.mv(det.cam.acquire_time, time)

Supporting negative time to count against a monitor detector is only supported by EpicsScaler and ScalerCH in the underlying EPICS record support.

Steps to Reproduce (for bugs)

n/a

Context

Converting existing APS SPEC macros to BlueSky and this is a barrier. Many of these macros pass the counting time as a parameter.

Your Environment

 'BLUESKY_VERSION': '1.0.0.post2+gf124cf7',
 'OPHYD_VERSION': '1.0.0',
@prjemian
Copy link
Contributor Author

The time argument could (should) be staged to the detector stage_sigs for the bp.count(). Then removed or restored to the previous value after bp.count() is finished. That should happen as part of bp.count().

@danielballan
Copy link
Member

danielballan commented Feb 26, 2018

This has come up a couple times, and we've resisted so far. I'll explain why, and you can decide whether or not you are convinced. :- )

In simpler times, count time was just one parameter and it was common to configure all detectors alike. But now, different detectors parameterize count time differently. Some simple point detectors still have a simple exposure time; but Area Detector specifies exposure time using three parameters.

If plans are to generically configure a 'count time', ophyd would need to present some standard API for it to do so, and it's not really clear to us what this API should be. Moreover, it's common to need to configure different detectors differently, so a time parameter passed to the plan is too blunt an instrument. We judged that it was better to teach users one way to do this that will work in the general case: configure the devices individually either via abs_set/mv in a custom plan or interactively in IPython before running the plan.

Where a time parameter is desired, we think the right solution is to implement specific plans bound to specific hardware, accounting for the hardware's individual concept of "count time". Of course, in general we try to avoid needing to make custom plans for specific hardware... in this case we judged that the cure would be worse than the disease. If you think it can be done, the way forward would be to come up with an optional ophyd interface to 'count_time' that devices could implement.

@prjemian
Copy link
Contributor Author

I'm not convinced.

Seems like the way forward is to write a set_count_time(det, time) function (or set_count_time(det_list, time)). The user could always do differently but this is the obvious next step. I'll take this on as an adjunct to per-axis bespoke tuning.

extremely strong opinion here

At the end of the day, it is the responsibility of the data acquisition system to move movables and to read readables. One part of reading (counting from detectors with variable acquisition time) is to specify for how long. Just because this specification has become a complex decision process does not remove the responsibility. We should not push the process of how to set the counting time to the user. Either we introduce a standard interface to which all such detectors adhere, or we introduce a function that overcomes the complexity.

@danielballan
Copy link
Member

danielballan commented Feb 28, 2018

We've been talking through this this afternoon. You have convinced @tacaswell and me that bluesky/ophyd can take a more "batteries included" approach toward setting count times.

We'd like more consideration and discussion on whether the pre-assembled plans (count, scan, etc.) should accept this responsibility. It seems to have benefits and costs, which we'll defer to future discussion.

You mentioned introducing "a function that overcomes the complexity." Let's start with that. We propose this way forward:

  1. Expose one consistent specially-named Signal, which for complex Devices may be coupled to multiple other Signals (for AD: acquire_time, exposure_time, acquire_period), on all relevant detectors in ophyd.
  2. Improve the bluesky.plans_stubs.configure_count_time plan stub bluesky.preprocessors.configure_count_time_wrapper as needed to work with this special Signal in a generalizable way.

One important consideration here is that changing the count time on a device should notify the RunEngine that the any previously issued EventDescriptor(s) referring to that device should be reissued next time they are read, so that they can record the updated configuration. (Exposure time is typically recorded in the Event Descriptor.) The configure_count_time plan does this, by using Msg('configure'). We have more thoughts on how to improve this, not directly germane to this issue.

One convention in common use is that positive time is in seconds, negative time is in monitor (detector) counts which means the monitor detector must be declared in advance.

Can we make this a separate discussion? This seems, to us, to not generalize beyond Scalers.

@prjemian
Copy link
Contributor Author

prjemian commented Mar 9, 2018

Thanks for the consideration.

Agree the count against monitor feature can go separate. It seems to be specific to Scaler devices.

So, a detector with the proposed SignalCountTimeMixin (my name) will have a count_time attribute that is then handled by configure_count_time ()? Why not build that into the mixin as a method?

@danielballan
Copy link
Member

danielballan commented Mar 13, 2018

Why not build that into the mixin as a method?

Just to make sure we're on the same page:

I proposed one or more specially-named Components. A plan would look for them and set them if they exist on a given device:

class MyDetector:
    count_time = Cpt(EpicsSignal, '...')
    acquire_period = cpt(EpicsSignal, '...')

def configure_count_time(device, count_time, acquire_period=None):
    # ^ That signature is just an initial suggestion.
    # It's probably more complicated!
    if hasattr(device, 'count_time'):
        yield from abs_set(device.count_time, count_time)
    if acquire_period is not None and hasattr(device, 'acquire_period'):
        yield from abs_set(device.acquire_period, acquire_period)

And I think you are proposing a specially-named method:

class Mixin:
    def configure_count_time(self, ...):
        ...
def configure_count_time(device, count_time, acquire_period=None):
    yield Msg('configure_count_time', device, count_time, acquire_period)

For plans to access that method, we would need to add a bunch more stuff:

  • RunEngine._configure_count_time, which calls device.configure_count_time
  • a corresponding command (message type) registered with the RunEngine, like Msg('configure_count_time')
  • plan(s) that yield that message

... which is not necessarily a bad thing! Is that what you have in mind? A new kind of "command" distinct from the generic "set" command, accessing a special new method opposed to a the set() method of a certain Component or Components?

@danielballan
Copy link
Member

We have documented why we do not build in a concept of exposure time to count with input and sign off from @prjemian in #1223 so I think it is safe to close this.

There are contexts where specifying one time for a count does make sense, but it should happen in a higher layer, a plan that wraps the fundamental count plan and can make more assumptions about what the user wants.

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

No branches or pull requests

2 participants