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

Raise on get/set of Abstract parameters, rework example notebook #3217

Merged
merged 8 commits into from
Jul 19, 2021

Conversation

jenshnielsen
Copy link
Collaborator

@jenshnielsen jenshnielsen commented Jul 19, 2021

  • Move notebook to a subdir where it belongs
  • Raise on get/set of abstract parameters (until we can re enable the error on init) add a test too
  • Remove sections about error on init from notebook
  • Fix a typo in the title of the notebook
  • Enable execution of notebook so this can be avoided in the future

@jenshnielsen jenshnielsen force-pushed the abstract_notebook_move branch from b6c3ae4 to a7bab03 Compare July 19, 2021 12:24
@jenshnielsen jenshnielsen enabled auto-merge July 19, 2021 12:25
@jenshnielsen jenshnielsen added this to the 0.27.0 milestone Jul 19, 2021
@jenshnielsen jenshnielsen force-pushed the abstract_notebook_move branch from a7bab03 to 4dc791d Compare July 19, 2021 12:29
@astafan8 astafan8 changed the title Abstract parameter updates Raise on get/set of Abstract parameters, rework example notebook Jul 19, 2021
@codecov
Copy link

codecov bot commented Jul 19, 2021

Codecov Report

Merging #3217 (3015f6f) into master (dc34301) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #3217   +/-   ##
=======================================
  Coverage   65.92%   65.92%           
=======================================
  Files         218      218           
  Lines       28939    28943    +4     
=======================================
+ Hits        19078    19082    +4     
  Misses       9861     9861           

@jenshnielsen jenshnielsen merged commit 5ef883f into microsoft:master Jul 19, 2021
bors bot added a commit that referenced this pull request Jan 8, 2022
3718: Reimplement abstract instrument r=jenshnielsen a=jenshnielsen

This is a start of re implementing the abstract parameter interface reverted in #3197 and #3217 based on top of #3696

Compared to the solution based on init subclass this works a bit differently. The metaclass in use is only implemented for the Instrument class but not for the InstrumentChannel. Changing this would require metaclass inheritance which is a bad idea. 
This meas that it is not an error to create an InstrumentChannel with abstract parameters but it is an error to attach such a thing to an instrument at instrument creation time. I don't think this is a big issue since that is how most instruments are created. 

Still need to update docs again and verify if there are other reverted changes to bring over 



Co-authored-by: Jens H. Nielsen <[email protected]>
@jenshnielsen jenshnielsen deleted the abstract_notebook_move branch October 25, 2024 11:50
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.

2 participants