Give InstrumentBase a label that can be nicely formatted
Add a label to InstrumentBase that can store a nicely-formatted instrument name. The implementation is similar to the label in Parameter. Previously discussed here.
- [x] Make sure that the pull request contains a short description of the changes made.
- [x] If you are submitting a new feature please document it. This can be in the form of inline docstrings, an example notebook or restructured text files.
- [x] Please include automatic tests for the changes made when possible.
- [x] Create a file in the docs\changes\newsfragments folder with a short description of the change.
The order of the arguments in the __init__ is a little bit unintuitive (especially when compared to the Parameter class). I wanted to leave the order of the existing arguments unchanged. I am not sure what your policy for breaking changes is. If preferred, I am happy to swap the order.
Please point me to any other parts of code that need to be changed accordingly (if any).
Codecov Report
Merging #4460 (968a4ed) into master (108b0c4) will increase coverage by
0.03%. The diff coverage is100.00%.
:exclamation: Current head 968a4ed differs from pull request most recent head c77d137. Consider uploading reports for the commit c77d137 to get more accurate results
@@ Coverage Diff @@
## master #4460 +/- ##
==========================================
+ Coverage 68.00% 68.03% +0.03%
==========================================
Files 299 290 -9
Lines 31440 31323 -117
==========================================
- Hits 21381 21311 -70
+ Misses 10059 10012 -47
According to codecov, the coverage in the file instrument_drivers/AlazarTech/ATS.py drops. I'm not sure why. Otherwise, the checks seem to pass.
@klkl0808 You can safely disregard that. There is a bug in codecov where it sometimes does not merge all the coverage from the different runners correctly (this test only runs on windows)
The order of the arguments in the init is a little bit unintuitive (especially when compared to the Parameter class). I wanted to leave the order of the existing arguments unchanged. I am not sure what your policy for breaking changes is. If preferred, I am happy to swap the order.
I agree that this order is less ideal but I am not comfortable changing it
I think this should also work for InstrumentModule / InstrumentChannel since they forward **kwargs but it would be nice with a test for that.
Did you think about if there is a need for something like full name for the label? E.g. such that you can merge the label of a channel and the instrument?
I think this should also work for
InstrumentModule/InstrumentChannelsince they forward**kwargsbut it would be nice with a test for that.
Done.
Did you think about if there is a need for something like full name for the label? E.g. such that you can merge the label of a channel and the instrument?
Good point. I think it makes sense, because the user could use the label in conjunction with the parent instrument name or standalone. So it would be good to have two versions of labels.
From a user perspective, it would make sense to set the label like instrument.channel.label = "new label". But there are two differences with respect to the name property. First, name is not settable. Second, name refers to the full_name. But in example above, label rather refers to the short_label. The other question is how to combine the labels of parent and child (which separator, word capitalization, length of label...?).
Since the idea of the label is to have a nice format, maybe it would be best to leave this up to the user. My suggestion would be to give each InstrumentBase a short_label and a long_label. They are equal by default and can both be set by the user. In an InstrumentChannel, the user can encode information about the parent in the long_label. This is reasonable in my opinion, because each InstrumentChannel has exactly one parent.
@klkl0808 Thanks, I think its fair that we wait with figuring out if merging needs to happen later