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

Give InstrumentBase a label that can be nicely formatted #4460

Merged
merged 5 commits into from
Aug 24, 2022

Conversation

klkl0808
Copy link
Contributor

@klkl0808 klkl0808 commented Aug 2, 2022

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.

  • Make sure that the pull request contains a short description of the changes made.
  • 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.
  • Please include automatic tests for the changes made when possible.
  • Create a file in the docs\changes\newsfragments folder with a short description of the change.

@klkl0808
Copy link
Contributor Author

klkl0808 commented Aug 2, 2022

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).

@klkl0808 klkl0808 marked this pull request as ready for review August 2, 2022 16:26
@codecov
Copy link

codecov bot commented Aug 3, 2022

Codecov Report

Merging #4460 (968a4ed) into master (108b0c4) will increase coverage by 0.03%.
The diff coverage is 100.00%.

❗ 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     

@klkl0808
Copy link
Contributor Author

klkl0808 commented Aug 3, 2022

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.

@jenshnielsen
Copy link
Collaborator

@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)

docs/changes/newsfragments/4460.new Outdated Show resolved Hide resolved
@jenshnielsen
Copy link
Collaborator

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?

@klkl0808
Copy link
Contributor Author

klkl0808 commented Aug 3, 2022

I think this should also work for InstrumentModule / InstrumentChannel since they forward **kwargs but 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.

@jenshnielsen
Copy link
Collaborator

@klkl0808 Thanks, I think its fair that we wait with figuring out if merging needs to happen later

@jenshnielsen jenshnielsen merged commit e214ca1 into microsoft:master Aug 24, 2022
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.

None yet

3 participants