-
Notifications
You must be signed in to change notification settings - Fork 87
Add CompareStatus, TransitionStatus, AllAndStatus and OrAnyStatus #1265
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
28b80c9 to
427d0cc
Compare
|
Hi @ZLLentz, the third and biggest one with the StatusObjects presented during the workshop. Again, let me know if anything should be adapted. |
427d0cc to
5752e6c
Compare
ophyd/status.py
Outdated
| Parameters | ||
| ---------- | ||
| signal: The device signal to compare. |
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.
Please use numpy-style docstrings.
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.
done
ophyd/status.py
Outdated
| value: float | int | str, | ||
| *, | ||
| operation: Literal["==", "!=", "<", "<=", ">", ">="] = "==", | ||
| raise_exc_value: float | int | str | list[float | int | str] | None = None, |
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.
Can we call this failure_value? That is raises an exception is an implementation detail not its purpose.
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.
done
ophyd/status.py
Outdated
|
|
||
| def _compare_callback(self, value, **kwargs) -> bool: | ||
| """Callback for subscription status""" | ||
| if value in self._raise_exc_values: |
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.
how does this check (which is always ==) interact with greater / less that tests?
Would it make sense to say "The device has failed if the value goes above X"?
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.
We adapted the code, renamed operation to operation_success, and added operation_failure
ophyd/status.py
Outdated
| Parameters | ||
| ---------- | ||
| signal: The device signal to monitor. |
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.
same comment as above, please use numpy style docstrings.
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.
done
|
|
||
| def __init__( | ||
| self, | ||
| signal: Signal, |
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 remain a bit concerned about adding typing ad-hoc, but compared to the #362 I'm less concerned about bringing in complete typing on a new object.
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.
Typehints here seem to be useful, I would propose to keep them. Please let me know if you disagree.
| if isinstance(value, str): | ||
| if operation not in ("==", "!="): | ||
| raise ValueError( | ||
| f"Invalid operation: {operation} for string comparison. Must be '==' or '!='." | ||
| ) | ||
| if operation not in ("==", "!=", "<", "<=", ">", ">="): | ||
| raise ValueError( | ||
| f"Invalid operation: {operation}. Must be one of '==', '!=', '<', '<=', '>', '>='." | ||
| ) |
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.
Would it make sense to also check that the signal has length 1?
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.
Sure, it's true that the assumption is that the signal has length 1.
As this may easily get too complicated with signals returning numpy arrays or lists, we simply safeguard against the scenario and set the exception to the error that results from the comparison.
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.
Can we use signal.describe() on init? Not bullet proof, but I think it may be less messy than trying to do the typechecking later.
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.
"this is too much trouble, let ducktyping fail" is a fine answer.
ophyd/status.py
Outdated
| transitions: list[float | int | str], | ||
| *, | ||
| strict: bool = True, | ||
| raise_states: list[float | int | str] | None = None, |
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.
same as above, I think failure_states or similar is a clearer name.
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.
done
ophyd/status.py
Outdated
| if not isinstance(transitions, list): | ||
| raise ValueError( | ||
| f"Transitions must be a list of values. Received: {transitions}" | ||
| ) | ||
| self._transitions = transitions |
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.
| if not isinstance(transitions, list): | |
| raise ValueError( | |
| f"Transitions must be a list of values. Received: {transitions}" | |
| ) | |
| self._transitions = transitions | |
| self._transitions = tuple(transitions) |
If we want to do type checking we should check against Sequence (and the typing should be relaxed) but I think it is better to normalize the input through tuple and let Python handle the type checking for us.
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.
done
ophyd/status.py
Outdated
| return self._is_finished() | ||
|
|
||
| def _is_finished(self) -> bool: | ||
| """Check if the status is finished""" | ||
| return self._index >= len(self._transitions) |
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.
Nitpick: this one-line function seems like it's only used in one place and I'm not sure if it makes the code more readable or not.
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.
done
ophyd/status.py
Outdated
| for st in self.status_list: | ||
| with st._lock: | ||
| if st.done and not st.success: | ||
| self.set_exception(st.exception()) # st._exception |
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 wonder if there's any way to leverage ExceptionGroup here, but maybe not if we must be done and failed when any status has failed.
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.
The issue is that Python 3.8 is not supporting ExceptionGroup yet.
ophyd/status.py
Outdated
| combined_exceptions = RuntimeError( | ||
| "; ".join(f"{type(exc).__name__}: {exc}" for exc in exceptions) | ||
| ) |
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.
This one can definitely be an ExceptionGroup
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.
same as above
ophyd/status.py
Outdated
| with st._lock: | ||
| st.add_callback(inner) | ||
|
|
||
| def set_exception(self, exc): |
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 notice these two new statuses override set_exception while nothing else does. What's the purpose of the override here?
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.
This is a remnant from trying to set the exception on the sub-statuses. I removed it.
| ) | ||
| return False | ||
| if self._index == 0: | ||
| if value == self._transitions[0]: |
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.
Should this have some checking that the first change we see is to the first state in strict mode?
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.
Hej,
we realized that the usage of strict was not properly documented. We have added a section with examples in the notes section of the docstring. In short, the strict option does not imply that an exception will be raised, it simply decides whether a status resolves in success or not. To mark states for exception, we would expect that failure_states should be used. Please let us know what you think about the section in the docstring.
class TransitionStatus(SubscriptionStatus):
"""
Status class to monitor transitions of a signal value through a list of specified transitions.
The status is finished when all transitions have been observed in order. The keyword argument
`strict` determines whether the transitions must occur in strict order or not. The strict option
only becomes relevant once the first transition has been observed.
If `failure_states` is provided, the status will raise an exception if the signal value matches
any of the values in `failure_states`.
Parameters
----------
signal: Signal
The device signal to monitor.
transitions: list
A list of values to transition through.
strict: bool, optional
Whether the transitions must occur in strict order. Defaults to True.
failure_states: list, optional
A list of values that will raise an exception if encountered. Defaults to None.
run: bool, optional
Whether to run the status callback on creation or not. Defaults to True.
event_type: optional
The type of event to trigger on transition. Defaults to None (default sub).
timeout: float | None, optional
The timeout for the status. Defaults to None (indefinite).
settle_time: float, optional
The time to wait for the signal to settle before checking transitions. Defaults to 0.
Notes
-----
The 'strict' option does not raise if transitions are observed which are out of order.
It only determines whether a transition is accepted if it is observed from the
previous value in the list of transitions to the next value.
For example, with strict=True and transitions=[1, 2, 3], the sequence
0 -> 1 -> 2 -> 3 is accepted, but 0 -> 2 -> 1 -> 3 is not and the status will not complete.
With strict=False, both sequences are accepted.
However, with strict=True, the sequence 0 -> 1 -> 3 -> 1 -> 2 -> 3 is accepted.
To raise an exception if an out-of-order transition is observed, use the
`failure_states` keyword argument.
"""
ophyd/status.py
Outdated
|
|
||
| class AndAllStatus(DeviceStatus): | ||
| """ | ||
| A status that combines mutiple status objects in a list using logical and. |
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.
As above, please use numpydoc style docstrings.
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.
done
ophyd/status.py
Outdated
| if self.done: | ||
| return | ||
|
|
||
| for st in self.status_list: |
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 loop through the list? Isn't the status object passed in the most recently changed one and we can just react to 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.
done
ophyd/status.py
Outdated
| A status that combines mutiple status objects in a list using logical and. | ||
| The status is finished when all status objects in the list are finished. | ||
| If any status object fails, the combined status will also fail and | ||
| set the exception from the first failed status on all sub-statuses. |
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 do not see where the exception is being pushed down to the children, am I missing something?
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.
Sorry, this is a leftover sentence from an initial idea. I will remove this sentence.
Sub-statuses may have their own logic for set_exception/set_finished. As this logic may not necessarily properly check if the status is already done, we risk that they raise in a manner that may be confusing. To avoid this, we dropped the idea again and instead just call set_finished/set_exception on the AndAllStatus.
ophyd/status.py
Outdated
| return item in self.status_list | ||
|
|
||
|
|
||
| class OrAnyStatus(DeviceStatus): |
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.
What is the use case for "or"? . When we added AndStatus + __and__ we could not think of a good example of when it would be useful and chose to leave it out rather than go for the completionist approach.
If we add this, then we should also add __or__ to StatusBase which will in turn bring up my question of what this is adding over st1 or st2 or st3?
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 briefly tried the st1 or st2 or st3 syntax, and was positively surprised. I was not aware of it. The difference to AnyOrStatus is that in its current implementation, the AnyOrStatus will only result in an exception if all status objects are failed. I could see that this comes in handy in the future, although we currently do not have an immediate use case in action.
dev = Device(name='dev')
st1 = StatusBase()
st2 = StatusBase()
st3 = StatusBase()
any_or_st = AnyOrStatus(dev, [st1,st2,st3])
or_st = st1 or st2 or st3
st1.set_exception(ValueError("Test"))
any_or_st.done # False
or_st.done # True, success= False
st2.set_finished()
any_or_st.done # True, success = True
or_st.done # True, success = FalseThere 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.
As the behavior is different, I am not certain if overwriting __or__ in StatusBase is actually desired. Both scenarios seem to be valid use cases.
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.
If you do not over-ride __or__ python returns the first "true" object or the last object.
In [4]: or_st = st1 or st2 or st3
In [5]: or_st is st1
Out[5]: True
In [6]: type(or_st)
Out[6]: ophyd.status.StatusBase
so or is not doing what you want!
However, I had something twisted in my head, we can override & and | with __and__ and __or__
This actually makes the case to implement __or__ stronger to me as the default behavior "works", but is not at all the intended meaning.
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 see, sorry for not getting this before fully. With &, the AndStatus almost fully serves the purpose which we would like to see. I will take this into account when I split the PR, and take a look at how __or__ could be implemented.
|
I'll fix the linter later today. Note: I might be easier if the whole file complies to |
9029ec2 to
7011fe2
Compare
|
Could you split the compare/transition status and the and/or status into separate PRs? I suspect we will merge the first two faster than the second two. |
2e4e944 to
6096c1a
Compare
6096c1a to
4348dcd
Compare
Done! |
| def _compare_callback(self, value, **kwargs) -> bool: | ||
| """Callback for subscription status""" | ||
| try: | ||
| if isinstance(value, list): |
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.
isinstance checks like this are a bit of a trap as things like tuple will not hit this (and then checking for sequences require checking it is not a string, ...) and it gets messy.
Summary
This PR adds 4 new StatusObjects that simplify signal comparison, transitions or combining status objects.
CompareStatus
Status to compare updates from a signal. It supports float & int for a few operations ('==', '!=', '<', '<=', '>', '>='.) and strings for '==', '!='. A list of values can be specified for which an exception should be set on the status.
TransitionStatus
Status to define a transition of states that the signal needs to run through. It is possible to specify whether the transition should be strict. A list of values can be specified for which an exception should be set.
AndAllStatus
Similar to AndStatus, but a DeviceStatus and takes a list of status objects.
OrAnyStatus
Similar to OrStatus, but a DeviceStatus and takes a list of status objects.