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

Refactor States Devices #434

Open
ZLLentz opened this issue May 21, 2020 · 7 comments
Open

Refactor States Devices #434

ZLLentz opened this issue May 21, 2020 · 7 comments

Comments

@ZLLentz
Copy link
Member

ZLLentz commented May 21, 2020

Expected Behavior

  • Code should be clean and easy to follow
  • State returns should be unambiguous
  • State checks should be less error-prone

Current Behavior

  • String returns are inconsistent
  • Small changes in state lists can cause hard to track down bugs in seemingly unrelated classes
  • Internally we're juggling and comparing strings everywhere

Possible Solution

  • Make all state things represented by Enums (e.g. state.position should return an enum)
  • Refactor and simplify around this idea

Context

(even after much effort today/yesterday)

@klauer
Copy link
Contributor

klauer commented May 21, 2020

I'd wager you're already thinking of using string enums, but regardless a few points in favor of refactoring using them:

In [3]: class StateEnum(str, enum.Enum):
   ...:     state_a = 'state_a'
   ...:     state_b = 'state_b'
   ...:

In [4]: StateEnum.state_a == 'state_a'
Out[4]: True

In [5]: StateEnum['state_a']
Out[5]: <StateEnum.state_a: 'state_a'>

Raises ValueError with StateEnum('invalid_value'), keeps backward-compatibility with strings, is pretty readable, etc.

@ZLLentz
Copy link
Member Author

ZLLentz commented May 21, 2020

That's neat, I didn't know there were specialized Enum classes. Maybe we use a slightly customized CaseInsensitiveStringEnum...

@klauer
Copy link
Contributor

klauer commented May 21, 2020

Case insensitive option would be interesting, hmm

@ZryletTC
Copy link
Contributor

How does this apply to #412? The zoom and focus motors (at least for now) are not set up as StatePositioners.

@ZLLentz
Copy link
Member Author

ZLLentz commented May 21, 2020

I picked the wrong issue by accident, fixed

@ZLLentz
Copy link
Member Author

ZLLentz commented May 21, 2020

Case insensitivity would be used for something like:
y_states.move('yag')
Where you don't have to remember that it is actually YAG or Yag
The move would accept an enum or any matching int or string

@klauer
Copy link
Contributor

klauer commented Oct 15, 2021

The refactored solution should ensure #406 is addressed, with the exception text reproduced here:

[2020-03-31 14:20:34,112] [ERROR   ] - Subscription meta callback exception (EpicsSignal(read_pv='IM1L0:XTES:MMS:STATE:GET_RBV', name='im1l0_y_states_state', parent='im1l0_y_states', value=4, timestamp=1585229929.895677, auto_monitor=False, string=False, write_pv='IM1L0:XTES:MMS:STATE:SET', limits=False, put_complete=False))
Traceback (most recent call last):
  File "/reg/g/pcds/pyps/apps/dev/pythonpath/ophyd/ophydobj.py", line 471, in inner
    cb(*args, **kwargs)
  File "/reg/g/pcds/pyps/apps/dev/pythonpath/pcdsdevices/state.py", line 108, in _late_state_init
    self._state_init()
  File "/reg/g/pcds/pyps/apps/dev/pythonpath/ophyd/device.py", line 1607, in wrapped
    ret = func(self, *args, **kwargs)
  File "/reg/g/pcds/pyps/apps/dev/pythonpath/pcdsdevices/state.py", line 544, in _state_init
    super()._state_init()
  File "/reg/g/pcds/pyps/apps/dev/pythonpath/ophyd/device.py", line 1607, in wrapped
    ret = func(self, *args, **kwargs)
  File "/reg/g/pcds/pyps/apps/dev/pythonpath/pcdsdevices/inout.py", line 73, in _state_init
    self._extend_trans_enum(self.out_states, 1)
  File "/reg/g/pcds/pyps/apps/dev/pythonpath/pcdsdevices/inout.py", line 124, in _extend_trans_enum
    index = self.states_list.index(state)
ValueError: 'OUT' is not in list

and summarizing per @ZLLentz in that issue:

The standard in/out states in the motion library have "OUT" on their enum. So most things will be using "OUT". The special ones I made for the PPM and the XPIM use "Out" instead. I will change them all to "OUT" to increase consistency with states that came before these new systems.

(the case sensitivity considerations mentioned above)

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

3 participants