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

Split ChannelList into a ChannelTuple and a ChannelList #3851

Merged
merged 45 commits into from
Jan 24, 2022

Conversation

jenshnielsen
Copy link
Collaborator

@jenshnielsen jenshnielsen commented Jan 21, 2022

This creates a cleaner interface where the ChannelTuple does not contain modifying methods (which would just raise)

  • ChannelList now implementes the full MutableSequece ABC
  • All drivers that used lock have been updated to convert the channellist to a channeltuple
  • Instrument and station has been updated to allow channeltuples.
  • Code coverage of ChannelList/Tuple has been improved to almost 100 %

One could significantly cleanup the code by removing the whole lock functionality but that will need to be left till
the things has been deprecated for a while.

Question? Should we get rid of the whole _channels being a tuple or a list. That would significantly simplify the typing and remove the need for casting in the list implementation.

  • Update drivers as possible. Any driver that does not modify its channellists should probably use ChannelTuple
  • remove locking logic leftover from channeltuple
  • Fix update of channelmapping
  • Update tests to cover this. Let the DummyChannelInstrument use a ChannelTuple and rewrite test that modifies channellist to use a channellist. Cover new methods added, ensure that channelmapping logic is covered
  • Fix znb driver that modifies internal state of the ChannelList
  • Update docs
    Possible future improvements
  • Deprecate lock ? Probably not yet. We should first update contrib drivers
  • Can we make this more generic so myinstr.chanels is a ChannelTuple[Mychanneltype]

@jenshnielsen jenshnielsen force-pushed the channeltuple branch 2 times, most recently from c5e18f7 to 3c7d786 Compare January 21, 2022 18:42
@codecov
Copy link

codecov bot commented Jan 21, 2022

Codecov Report

Merging #3851 (a513359) into master (cb9184b) will increase coverage by 0.14%.
The diff coverage is 81.13%.

@@            Coverage Diff             @@
##           master    #3851      +/-   ##
==========================================
+ Coverage   65.66%   65.80%   +0.14%     
==========================================
  Files         228      228              
  Lines       31055    31056       +1     
==========================================
+ Hits        20392    20437      +45     
+ Misses      10663    10619      -44     

@jenshnielsen jenshnielsen self-assigned this Jan 23, 2022
@jenshnielsen jenshnielsen marked this pull request as ready for review January 24, 2022 08:31
Copy link
Contributor

@astafan8 astafan8 left a comment

Choose a reason for hiding this comment

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

Question? Should we get rid of the whole _channels being a tuple or a list. That would significantly simplify the typing and remove the need for casting in the list implementation.

as discussed offline, making _channels a list will make things easier. getting rid of it, i think, is possible, but then _channel_mapping becomes THE underlying structure, and i doubt that this will result in a better code. in fact, perhaps removing the _channel_mapping and generating it only when needed, could improve the code, but also might not

Deprecate lock ? Probably not yet. We should first update contrib drivers

agree. first update contrib drivers, but then, yes, deprecate the lock

Can we make this more generic so myinstr.chanels is a ChannelTuple[Mychanneltype]

i think this should be possible. _chan_type needs to become a type alias bound to InstrumentChannel and used in all the signatures. can be attempted in a separate PR :)

qcodes/instrument/channel.py Outdated Show resolved Hide resolved
qcodes/instrument/channel.py Outdated Show resolved Hide resolved
qcodes/instrument/channel.py Outdated Show resolved Hide resolved
qcodes/instrument/base.py Show resolved Hide resolved
qcodes/instrument/channel.py Outdated Show resolved Hide resolved
qcodes/instrument/channel.py Outdated Show resolved Hide resolved
qcodes/instrument/channel.py Outdated Show resolved Hide resolved
qcodes/instrument/channel.py Outdated Show resolved Hide resolved
qcodes/instrument/channel.py Outdated Show resolved Hide resolved
Co-authored-by: Mikhail Astafev <astafan8@gmail.com>
@astafan8
Copy link
Contributor

bors merge

@bors
Copy link
Contributor

bors bot commented Jan 24, 2022

🕐 Waiting for PR status (Github check) to be set, probably by CI. Bors will automatically try to run when all required PR statuses are set.

@bors bors bot merged commit 44c46fe into microsoft:master Jan 24, 2022
@jenshnielsen jenshnielsen deleted the channeltuple branch January 24, 2022 12:58
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

2 participants