-
Notifications
You must be signed in to change notification settings - Fork 3
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
Conversation
I appreciate the thorough breakdown you wrote in the description 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.
I think this is a good approach
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 mostly looks good, but I found one technical mistake and have a couple consistency suggestions.
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 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]] |
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: should this return docstring be changed to Iterable too?
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.
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
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 anAugmentedValue
protocol, which in python just means it's promising the returned value will have the same fields asAugmentedValue
. In reality the returned values areepicscorelibs.ca.dbr.<ca_datatype>
types, which subclass python primitives and ONLY have fields dictated by theformat
kwargAugmentedValue
interface, it's not guaranteed to. (tsk tsk)pyepics
gets this information from thecainfo
function, which callsPV.get_ctrlvars
under the hood. The metadata is then stored on thePV
object (I believe)p4p
seems to create a subclass of their ownNTBase
for each epics NT, and status/severity are stored as attributes on that objectType
class of their own to use throughout all thisIn 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
docs/pre-release-notes.sh
and created a pre-release documentation page