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

Use metadata from DB records to populate DataKeys #137

Closed
wants to merge 11 commits into from

Conversation

DiamondJoseph
Copy link
Contributor

No description provided.

@DiamondJoseph DiamondJoseph force-pushed the signal_metadata branch 2 times, most recently from 1920904 to 69e14b6 Compare March 11, 2024 14:12
@DiamondJoseph DiamondJoseph marked this pull request as ready for review March 11, 2024 15:42
Copy link
Collaborator

@coretl coretl left a comment

Choose a reason for hiding this comment

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

A couple of minor things, please could you make a ticket for doing the same for the PVA side so we get this in PandA too?

src/ophyd_async/epics/_backend/_aioca.py Outdated Show resolved Hide resolved
Comment on lines 42 to 45
"units",
"lower_alarm_limit",
"upper_alarm_limit",
"lower_ctrl_limit",
"upper_ctrl_limit",
"lower_disp_limit",
"upper_disp_limit",
"lower_warning_limit",
"upper_warning_limit",
"precision",
Copy link
Collaborator

Choose a reason for hiding this comment

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

@danielballan do we want all these to appear in the descriptor document?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm happy to turn down to just units, precision. Currently for my downstream purposes I turn lower_ctrl_limit into soft_limit_min for NeXus, so if we can keep just one that's the one I want.

Copy link
Member

Choose a reason for hiding this comment

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

Only units and precision are blessed in the schema, but IIRC the schema is open-ended (additional properties allowed) so you can add any that you need.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So:

  1. add everything and filter what is required downstream
  2. just control_limits as is more meaningful than alarm/display/warning
  3. no limits: isn't actually describing the data, but the device's limits and should be somewhere in device configuration?

I'm leaning towards 3: the DataKey is a Key to the Data. It requires a source, but otherwise does it really benefit from knowing the limits? They should be instead kicked up to the device and plan rather than data and documents

Copy link
Collaborator

Choose a reason for hiding this comment

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

PVA also defines the same set of limits:

The question here is what is useful downstream.

  • If I am capturing a diode PV, do I want the display limits that would be used as axis ranges for plotting?
  • If I am capturing a furnace setpoint PV, do I want the control limits that show I was close to the maximum temperature the device accepts?
  • If I am capturing a thermocouple PV, do I want the warning or alarm limits to show that the sample was melting?

For that last point, we currently capture severity with each value, but we questioned the usefulness of that as maybe the scan should abort if the sample got too hot?

Copy link
Contributor Author

@DiamondJoseph DiamondJoseph Mar 13, 2024

Choose a reason for hiding this comment

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

CA PVA
lower_alarm_limit valueAlarm.lowAlarmLimit
upper_alarm_limit valueAlarm.highAlarmLimit
lower_ctrl_limit control.limitLow
upper_ctrl_limit control.limitHigh
lower_disp_limit display.limitLow
upper_disp_limit display.limitHigh
lower_warning_limit valueAlarm.lowWarningLimit
upper_warning_limit valueAlarm.highWarningLimit

Presumably the correct equivalents?
Do we want to do some mapping to either/both to get something consistent?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, I think we should map both to the same, then add them to event model as optional.
However I only think we should do this for the useful fields...
@danielballan which fields do you think are useful?

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 we have nested objects, so:

  • limits.control.low and limits.control.high
  • limits.display.low and limits.display.high
  • limits.warning.low and limits.warning.high
  • limits.alarm.low and limits.alarm.high

src/ophyd_async/epics/_backend/_aioca.py Outdated Show resolved Hide resolved
src/ophyd_async/epics/_backend/_aioca.py Show resolved Hide resolved
src/ophyd_async/epics/_backend/_aioca.py Show resolved Hide resolved
@DiamondJoseph
Copy link
Contributor Author

[...] make a ticket for doing the same for the PVA side [...]

Captured as #146

@DiamondJoseph
Copy link
Contributor Author

Replaced by #367

@DiamondJoseph DiamondJoseph deleted the signal_metadata branch June 14, 2024 08:42
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.

4 participants