-
Notifications
You must be signed in to change notification settings - Fork 339
[wip] Generic instrument in parameter #7655
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?
[wip] Generic instrument in parameter #7655
Conversation
4f8cb7e to
7177cc8
Compare
|
|
||
|
|
||
| class ParameterBase(MetadatableWithName): | ||
| class ParameterBase(MetadatableWithName, Generic[_InstrumentType_co]): |
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.
Before merging this we need to decide which generic parameters the parameter class should take and which order they should be in. 3.13+ allows some generic arguments to be omitted but since there are no support for kwargs they must be included in order so the first argument should be the one that must be costumized the most often
Candidates are ParamDataType, ParamRawDataType and Instrument.
It probably makes sense that ParamDataType is the first since each Parameter instance may have a different datatype while the Instrument type typically only needs to be customized in a subclass.
Should we include ParamRawData type or just leave it as any (if we include it should it come before or after Instrument)
15fbd4e to
5410bf0
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #7655 +/- ##
==========================================
- Coverage 59.78% 58.76% -1.03%
==========================================
Files 352 352
Lines 31659 31666 +7
==========================================
- Hits 18927 18607 -320
- Misses 12732 13059 +327 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
f6b1528 to
f0b3cce
Compare
304ba93 to
6b16d13
Compare
| class ParameterBase(MetadatableWithName): | ||
| class ParameterBase( | ||
| MetadatableWithName, Generic[_ParameterDataTypeVar, _InstrumentType_co] | ||
| ): |
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 need to agree on this order of generic arguments before merging since changing this would be a breaking change for all Parameter usage and implementations.
Roughly there are three relevant generic parameters for an instrument.
- ParameterData type (input/output from get/set)
- InstrumentType (what self.instrument returns)
- RawParameterData. What is passed to self.write/ask etc
Here I propose that we add 1 and 2 but omit 3 for the time being.
This means that we cannot add 3 before
No description provided.