Add ParameterMixin for extending parameter functionality
This commit introduces a general-purpose ParameterMixin designed to allow the extension of QCoDeS Parameters. It enables the addition of new features (e.g., on-cache-change callbacks, interdependent updates, and state reset synchronization) across parameter types without requiring per-parameter customization. It does handle docstring updates, enforces naming and allows to declare compatibility.
It replaces #6700.
I think this is a great addition. I have written Parameter subclasses with similar patterns multiple times + several in my measurement code.
@microsoft-github-policy-service agree
Hi, after some time away, I’ve rebased and cleaned up this PR. All type checks (mypy, pyright) and tests pass on my end now. I also fixed up the test and requirements files.
There is one possible issue: in the mixin implementation, I’m dynamically patching the parameter cache’s _update_with method to add the desired callback. I used # type: ignore[method-assign] for this line. I believe this is the most straightforward way without touching the cache implementation itself, but please let me know if you’d prefer a different approach or if you see any risks with this pattern.
Let me know if there’s anything unclear or if you’d like changes! Sorry for the long wait - looking forward to your feedback!
Edit: Thinking about it, the parameter group logic currently lives in the reset mixin, but it could easily be refactored into its own mixin class if desired for future extensions or broader use. I’m happy to refactor or split things out, but I need some feedback to know if the mixin approach is actually wanted before putting more time in.
Hi @jenshielsen, I’d really appreciate feedback on whether this ParameterMixin approach is something you’d consider for QCoDeS core. If it’s not quite the right fit, no worries—just let me know.
Thanks so much for your time!
@DCEM Sorry for the lack of reply. How would you feel about adding the mixins to the qcodes.extensions module rather than the parameters module. This will give us a bit of time to prove out the idea and test it before committing to the Mixins.
After writing the ParameterMixin, I thought it could help reduce code redundancy and save people from having to reinvent the same solutions. I think it could be a worthwhile addition to QCoDeS, and I’m happy to put it wherever you prefer—or not add it, if you don’t think it’s a fit.
Just let me know how you’d like to proceed!
@DCEM I think something like the ParameterMixin is a good idea. I don't have the time to more detailed think about the design and how it would fit in which makes me think that if we add it as an extension then we have some time to actually use it and test it in real world use cases before committing to extending the parameter modules api.
Hi @jenshnielsen,
Moving the mixin to extensions makes sense to me. Having it where it’s easy to use, test, and modify seems like a good choice, and it really is a modular parameter "extension". I’m happy to move the code there.
Two questions on my end:
-
For placement: should the mixin go as a single file in
qcodes/extensions,or would you prefer a subdirectory likeqcodes/extensions/parameter/orqcodes/extensions/parametermixin/? Or is there a different location you’d recommend? -
On implementation: to avoid touching the parameter cache code directly, I decided to wrap the parameter cache’s _update_with method dynamically (using # type: ignore[method-assign]). Do you see any risks with this approach, or would you prefer a different method?
Thanks again for your feedback!
@DCEM
- I don't have a strong preference but let's put is in a parameters subfolder. One file in that folder is fine thou if you prefer.
- If possible I prefer to avoid type ignores but I totally understand if you don't want to touch the cache too much. I think the current wrapper should be fine. It may be possible to get pyright (but perhaps not mypy) to type check with something like this. I will have a look at some point
Codecov Report
:x: Patch coverage is 99.61089% with 1 line in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 59.72%. Comparing base (1338ec9) to head (178643c).
:warning: Report is 34 commits behind head on main.
| Files with missing lines | Patch % | Lines |
|---|---|---|
| ...ions/parameters/parameter_mixin_on_cache_change.py | 97.56% | 1 Missing :warning: |
Additional details and impacted files
@@ Coverage Diff @@
## main #6731 +/- ##
==========================================
+ Coverage 59.39% 59.72% +0.33%
==========================================
Files 342 347 +5
Lines 30965 31222 +257
==========================================
+ Hits 18391 18647 +256
- Misses 12574 12575 +1
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
:rocket: New features to boost your workflow:
- :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
Hi, thanks for your reply.
I’ve moved the code as discussed. Both mypy and pyright show no issues, and I’ve (hopefully) fixed the pre-commit problems and have reread the commit guide in the process. It has been a while since i have been working on this, and I don't think i have read this version of it, sorry about that. I might still have missed something since it is somewhat extensive.
The documantation is missing right now - I will have a look on how to work with the .rst files.
edit: I updated the docstring to not cause issues with sphinx - if this PR needs more work, please let me know.
edit2: I thought about about the type ignore, do you think one of the follwing would be preferable:
- Instead of replacing the method on an instance, subclass the cache, override the method, and use subclass.
- modify the chache to provide a hook
@DCEM Running out of time for review now but will continue. The type checking errors that you are seeing in CI are fixed on main so merge / rebase main should resolve it.
I resolved the CI issues that came up for Python 3.12 (I had only been testing with 3.11 locally). After that I clicked “Update branch”, which I probably shouldn’t have done. That introduced a merge commit and disabled auto-merge, so I rebased on top of main and force-pushed to fix it.
Sorry for the inconvenience - I’ll try to improve my git workflow going forward.
@DCEM No worries (sorry about the approve and run the permission management is a bit more restrictive than I would like)
@DCEM No worries (sorry about the approve and run the permission management is a bit more restrictive than I would like)
Thank you - I think it’s good to be cautious. I’ll try to replicate the CI checks across all supported Python versions in the future so these kinds of issues can hopefully be minimized.
I also want to improve the documentation with Sphinx, but I need to figure out a better workflow for that.