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

DelegateParameter infer methods to find root parameters, instruments, and channels #5998

Merged
merged 15 commits into from Apr 30, 2024

Conversation

samantha-ho
Copy link
Contributor

@samantha-ho samantha-ho commented Apr 23, 2024

This PR adds methods to recursively search a chain of DelegateParameters with the aim of finding the root parameter. This also allows users to infer the InstrumentModule or Instrument that a chain of DelegateParameters stems from.

These methods also allow the user to customize which attributes of a Parameter link to other parameters.

@samantha-ho samantha-ho requested a review from a team as a code owner April 23, 2024 21:59
Copy link

codecov bot commented Apr 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.48%. Comparing base (51fcfa8) to head (9bee368).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5998      +/-   ##
==========================================
+ Coverage   67.43%   67.48%   +0.04%     
==========================================
  Files         349      350       +1     
  Lines       30211    30296      +85     
==========================================
+ Hits        20374    20445      +71     
- Misses       9837     9851      +14     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@samantha-ho
Copy link
Contributor Author

While writing tests, I realized there is a possibility for a DelegateParameter "loop" which would cause both get_root_parameter and get_parameter_chain to recurse infinitely.

There are certainly ways I could stop this, but I'm not sure the case is likely enough to bother with at this time. I invite other perspectives on the matter.

@astafan8
Copy link
Contributor

There are certainly ways I could stop this, but I'm not sure the case is likely enough to bother with at this time. I invite other perspectives on the matter.

I think it’d be useful to prevent the loops or at least error out when loops are discovered because that will be a welcoming and helpful user experience, so I’d encourage to do it

src/qcodes/parameters/infer.py Outdated Show resolved Hide resolved
src/qcodes/parameters/infer.py Outdated Show resolved Hide resolved
src/qcodes/parameters/infer.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@jenshnielsen jenshnielsen left a comment

Choose a reason for hiding this comment

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

Left some comments inline

src/qcodes/parameters/infer.py Outdated Show resolved Hide resolved
src/qcodes/parameters/infer.py Outdated Show resolved Hide resolved
src/qcodes/parameters/infer.py Outdated Show resolved Hide resolved
src/qcodes/parameters/infer.py Outdated Show resolved Hide resolved
src/qcodes/parameters/infer.py Outdated Show resolved Hide resolved
src/qcodes/parameters/infer.py Outdated Show resolved Hide resolved
src/qcodes/parameters/infer.py Outdated Show resolved Hide resolved
@samantha-ho
Copy link
Contributor Author

There are certainly ways I could stop this, but I'm not sure the case is likely enough to bother with at this time. I invite other perspectives on the matter.

I think it’d be useful to prevent the loops or at least error out when loops are discovered because that will be a welcoming and helpful user experience, so I’d encourage to do it

@samantha-ho samantha-ho reopened this Apr 25, 2024
samantha-ho and others added 5 commits April 25, 2024 10:23
- Move to extensions module
- Less verbose members of InferAttrs
- rename to infer_instrument_module
- Fix legacy import locations
Fix bug from adding loop detection
Move tests
@jenshnielsen jenshnielsen added this pull request to the merge queue Apr 30, 2024
Merged via the queue into microsoft:main with commit 2ce2148 Apr 30, 2024
20 checks passed
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