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

Units signal metadata #367

Merged
merged 33 commits into from
Jun 13, 2024
Merged

Units signal metadata #367

merged 33 commits into from
Jun 13, 2024

Conversation

abbiemery
Copy link
Collaborator

@abbiemery abbiemery commented Jun 5, 2024

Fixes #146 . Rebase of #137 .

Propagates units and precision metadata (if present) from PVA or CA signal backends.

@abbiemery abbiemery marked this pull request as ready for review June 7, 2024 09:42
@abbiemery
Copy link
Collaborator Author

Are we waiting on PVA changes for this, or that a different PR?

DiamondJoseph and others added 4 commits June 13, 2024 10:34
* unprivate variable

* Add limits handling for PVA and CA

* Tests for limits

* Limits are only ever type float

* Only add limits if set, used TypedDict

* Revert nan, add more tests

* Add limits and warnings to ioc
Co-authored-by: DiamondJoseph <53935796+DiamondJoseph@users.noreply.github.com>
src/ophyd_async/epics/_backend/_aioca.py Outdated Show resolved Hide resolved
src/ophyd_async/epics/_backend/_aioca.py Outdated Show resolved Hide resolved
src/ophyd_async/epics/_backend/_aioca.py Outdated Show resolved Hide resolved
Comment on lines 84 to 111
def _limits_from_value(value: Value) -> Limits:
valueAlarm = getattr(value, "valueAlarm", None)
_empty_limit = LimitPair(low=None, high=None)

def get_control_or_display_limit(limit: str) -> LimitPair:
if (control_or_display := getattr(value, limit, None)) is None:
return _empty_limit
low = getattr(control_or_display, "limitLow", None)
high = getattr(control_or_display, "limitHigh", None)
return LimitPair(
low=None if isnan(low) else low, high=None if isnan(high) else high
)

def get_alarm_or_warning_limit(limit: str) -> LimitPair:
if valueAlarm is None:
return _empty_limit
low = getattr(valueAlarm, f"low{limit}Limit", None)
high = getattr(valueAlarm, f"high{limit}Limit", None)
return LimitPair(
low=None if isnan(low) else low, high=None if isnan(high) else high
)

return Limits(
alarm=get_alarm_or_warning_limit("Alarm"),
control=get_control_or_display_limit("control"),
display=get_control_or_display_limit("display"),
warning=get_alarm_or_warning_limit("Warning"),
)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be written more simply?

Suggested change
def _limits_from_value(value: Value) -> Limits:
valueAlarm = getattr(value, "valueAlarm", None)
_empty_limit = LimitPair(low=None, high=None)
def get_control_or_display_limit(limit: str) -> LimitPair:
if (control_or_display := getattr(value, limit, None)) is None:
return _empty_limit
low = getattr(control_or_display, "limitLow", None)
high = getattr(control_or_display, "limitHigh", None)
return LimitPair(
low=None if isnan(low) else low, high=None if isnan(high) else high
)
def get_alarm_or_warning_limit(limit: str) -> LimitPair:
if valueAlarm is None:
return _empty_limit
low = getattr(valueAlarm, f"low{limit}Limit", None)
high = getattr(valueAlarm, f"high{limit}Limit", None)
return LimitPair(
low=None if isnan(low) else low, high=None if isnan(high) else high
)
return Limits(
alarm=get_alarm_or_warning_limit("Alarm"),
control=get_control_or_display_limit("control"),
display=get_control_or_display_limit("display"),
warning=get_alarm_or_warning_limit("Warning"),
)
def _limits_from_value(value: Value) -> Limits:
def get_limits(substucture_name: str, infix: str = "") -> LimitPair:
substructure = getattr(value, substucture_name, None)
low = getattr(substructure, f"low{infix}Limit", None)
high = getattr(substructure, f"high{infix}Limit", None)
return LimitPair(
low=None if isnan(low) else low, high=None if isnan(high) else high
)
return Limits(
alarm=get_limits("valueAlarm", infix="Alarm"),
control=get_limits("control"),
display=get_limits("display"),
warning=get_limits("valueAlarm", infix="Warning"),
)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately the suggestion fails quite a number of tests but i'll have a look.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not quite, it's low{infix}Limit but limitLow for the others.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I am inclined to leave it as it is.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about?

Suggested change
def _limits_from_value(value: Value) -> Limits:
valueAlarm = getattr(value, "valueAlarm", None)
_empty_limit = LimitPair(low=None, high=None)
def get_control_or_display_limit(limit: str) -> LimitPair:
if (control_or_display := getattr(value, limit, None)) is None:
return _empty_limit
low = getattr(control_or_display, "limitLow", None)
high = getattr(control_or_display, "limitHigh", None)
return LimitPair(
low=None if isnan(low) else low, high=None if isnan(high) else high
)
def get_alarm_or_warning_limit(limit: str) -> LimitPair:
if valueAlarm is None:
return _empty_limit
low = getattr(valueAlarm, f"low{limit}Limit", None)
high = getattr(valueAlarm, f"high{limit}Limit", None)
return LimitPair(
low=None if isnan(low) else low, high=None if isnan(high) else high
)
return Limits(
alarm=get_alarm_or_warning_limit("Alarm"),
control=get_control_or_display_limit("control"),
display=get_control_or_display_limit("display"),
warning=get_alarm_or_warning_limit("Warning"),
)
def _limits_from_value(value: Value) -> Limits:
def get_limits(substucture_name: str, low_name: str = "limitLow", high_name: str = "limitHigh") -> LimitPair:
substructure = getattr(value, substucture_name, None)
low = getattr(substructure, low_name, None)
high = getattr(substructure, high_name, None)
return LimitPair(
low=None if isnan(low) else low, high=None if isnan(high) else high
)
return Limits(
alarm=get_limits("valueAlarm", "lowAlarmLimit", "highAlarmLimit"),
control=get_limits("control"),
display=get_limits("display"),
warning=get_limits("valueAlarm", "lowWarningLimit", "highWarningLimit"),
)

Copy link
Collaborator Author

@abbiemery abbiemery Jun 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still no. Is this worth blocking on? Can I make an issue to put in the upcoming sprint to refactor it, it would be a good first issue.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got there in the end (assuming this passes), made a minor change to getattr(foo, bar, nan) as isnan(None) becomes unhappy. I'm assuming that if the attr is there (substructure is not None and hasattr(substructure, attr)) it will never be None

src/ophyd_async/epics/_backend/_p4p.py Outdated Show resolved Hide resolved
src/ophyd_async/epics/_backend/_p4p.py Outdated Show resolved Hide resolved
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

Successfully merging this pull request may close these issues.

Extract metadata from PVA Signals to make rich DataKeys for those signals
3 participants