pcdshub / pcdsdevices

Collection of Ophyd device subclasses for IOCs unique to LCLS PCDS.
https://pcdshub.github.io/pcdsdevices/
Other
5 stars 58 forks source link

Add TprTrigger default timing mode #1242

Closed slactjohnson closed 3 months ago

slactjohnson commented 4 months ago

Description

Make LCLS2 the default timing mode for the TprTrigger class.

Motivation and Context

closes #1241

How Has This Been Tested?

Tested interactively.

Where Has This Been Documented?

This PR.

Screenshots (if appropriate):

image

Pre-merge checklist

slactjohnson commented 4 months ago

Hmm, looks like Happi may have similar trouble with this enum: image image

tangkong commented 4 months ago

Yeah happi wouldn't know to interpret "TimingMode.LCLS1" as an enum it needs to import, so I guess we definitely need some type of string parsing here. Maybe something like:

def __init__(self, prefix, *, channel, name, timing_mode: TimingMode | str =TimingMode.LCLS2, **kwargs):
    if isinstance(timing_mode, str):
        try:
            timing_mode = getattr(TimingMode, timing_mode)
        except AttributeError as ex:
            raise ValueError(
                'Provided timing mode not recognized, please choose from: '
                f'{[mode.name for mode in TimingMode]}'
            )
    ...     
ZLLentz commented 4 months ago

Enums have built-in tools for consuming strings and returning enum values:

In [1]: from pcdsdevices.tpr import TimingMode

In [2]: TimingMode["LCLS1"]
Out[2]: <TimingMode.LCLS1: 1>

In [3]: TimingMode["LCLS2"]
Out[3]: <TimingMode.LCLS2: 2>

In [4]: TimingMode(1)
Out[4]: <TimingMode.LCLS1: 1>

In [5]: TimingMode(2)
Out[5]: <TimingMode.LCLS2: 2>

This makes it very easy to convert str and int inputs into the enum members.

These each raise a ValueError if the input isn't valid.

ZLLentz commented 4 months ago

One more mini-tip: the call/int version also accepts the enums, reducing the number of cases to check:

In [6]: TimingMode(TimingMode.LCLS1)
Out[6]: <TimingMode.LCLS1: 1>

In [7]: TimingMode(TimingMode.LCLS2)
Out[7]: <TimingMode.LCLS2: 2>
slactjohnson commented 4 months ago

My recent commit takes advantage of the tricks @ZLLentz mentioned, and now makes specifying timing_mode much more flexible while still erroring out on an invalid setting.

image

image

I kept running into errors with the type hints in Robert's example and I wasn't type hint savvy enough to figure it out.

image

tangkong commented 4 months ago

Ah that's my bad, I kinda just pseudocoded and didn't actually run the code myself. You probably have to use the explicit typing.Union

from typing import Union

...

def __init__(self, prefix, *, channel, name, timing_mode: Union[TimingMode, str]  = TimingMode.LCLS2, **kwargs):

Of course you could also leave it out as you have.

slactjohnson commented 3 months ago

I think I'm happy enough with this as is.