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

ENH: Unify data handling under EpicsData #73

Merged
merged 8 commits into from
Aug 13, 2024

Conversation

tangkong
Copy link
Contributor

@tangkong tangkong commented Aug 5, 2024

Description

Standardize our data handling to create and expect EpicsData instead of backend-specific data types.
Adjust tests and Client methods to comply with this change

Motivation and Context

We weren't handling status and severity before, and we should do so in a unified way
closes #71

Various libraries handle this EPICS metadata differently.

  • aioca defines an AugmentedValue protocol, which in python just means it's promising the returned value will have the same fields as AugmentedValue. In reality the returned values are epicscorelibs.ca.dbr.<ca_datatype> types, which subclass python primitives and ONLY have fields dictated by the format kwarg
    • This means that even though the return value supposedly implements the AugmentedValue interface, it's not guaranteed to. (tsk tsk)
  • pyepics gets this information from the cainfo function, which calls PV.get_ctrlvars under the hood. The metadata is then stored on the PV object (I believe)
  • p4p seems to create a subclass of their own NTBase for each epics NT, and status/severity are stored as attributes on that object
    • These NT classes also subclass python primitives
    • confusingly, they define a Type class of their own to use throughout all this

In the end I think this is the minimal amount of work to get us to the place we want. After we grab data from EPICS, we're dealing with normal data types stored in the superscore.model dataclasses, and won't be handling this custom shim layer anyway.

How Has This Been Tested?

Tests made to pass

Where Has This Been Documented?

This PR, and issue conversations

Pre-merge checklist

  • Code works interactively
  • Code follows the style guide
  • Code contains descriptive docstrings, including context and API
  • New/changed functions and methods are covered in the test suite where possible
  • Test suite passes locally
  • Test suite passes on GitHub Actions
  • Ran docs/pre-release-notes.sh and created a pre-release documentation page
  • Pre-release docs include context, functional descriptions, and contributors as appropriate

@tangkong tangkong marked this pull request as ready for review August 5, 2024 19:33
@ZLLentz
Copy link
Member

ZLLentz commented Aug 7, 2024

I appreciate the thorough breakdown you wrote in the description here

ZLLentz
ZLLentz previously approved these changes Aug 8, 2024
Copy link
Member

@ZLLentz ZLLentz left a comment

Choose a reason for hiding this comment

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

I think this is a good approach

Copy link
Collaborator

@shilorigins shilorigins left a comment

Choose a reason for hiding this comment

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

This mostly looks good, but I found one technical mistake and have a couple consistency suggestions.

superscore/control_layers/_aioca.py Show resolved Hide resolved
superscore/control_layers/_aioca.py Outdated Show resolved Hide resolved
superscore/control_layers/core.py Outdated Show resolved Hide resolved
superscore/client.py Outdated Show resolved Hide resolved
Copy link
Member

@ZLLentz ZLLentz left a comment

Choose a reason for hiding this comment

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

I don't see any functional issues beyond what has already been reviewed/fixed, looks good

@@ -87,20 +88,20 @@ def get(self, address: Union[str, list[str]]) -> Any:

Returns
-------
Any
Union[EpicsData, list[EpicsData]]
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: should this return docstring be changed to Iterable too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Man, this was really not my most detail oriented work haha. I'll pick it up in the next PR, to stop bothering you all with this one

@tangkong tangkong merged commit 463586c into pcdshub:master Aug 13, 2024
11 checks passed
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.

ENH: Record status and severity when reading PV values from EPICS
3 participants