Skip to content

Feature: modify existing quam + formatting#121

Merged
JacobFastQM merged 15 commits into
mainfrom
feature/modify-existing-quam
Jun 16, 2026
Merged

Feature: modify existing quam + formatting#121
JacobFastQM merged 15 commits into
mainfrom
feature/modify-existing-quam

Conversation

@JacobFastQM

@JacobFastQM JacobFastQM commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

Pull Request

Description

Customers use quam_builder package to initialize quam instance from scratch. Some have expressed the need to modify an existing quam instance when connections have been updated, qubits are added, etc. This feature adds the ability to add/remove qubit/channel/port from existing quam instances.

Type of Change

  • 🐛 Bug fix (non-breaking change which fixes an issue)
  • ✨ New feature (non-breaking change which adds functionality)
  • 💥 Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • 📚 Documentation update
  • 🔧 Refactoring (no functional changes)
  • ✅ Test addition/update
  • 🎨 Code style/formatting update

Checklist

Code Quality

  • My code follows the project's style guidelines
  • I have run pre-commit hooks and all checks pass (pre-commit run --all-files)
  • My commits follow the Conventional Commits format
  • I have performed a self-review of my code
  • My changes generate no new warnings

Testing

  • I have added tests that prove my fix/feature works
  • All new and existing tests pass locally (pytest)
  • I have tested my changes with the example scripts (if applicable)

Documentation

  • I have updated the documentation (if needed)
  • I have added docstrings to new functions/classes
  • I have updated the CHANGELOG.md (if applicable)
  • I have checked my code and corrected any misspellings

Does this PR introduce breaking changes? No

Testing Details

Unit tests in test_modify_quam.py

Reviewer Notes

I'm not sure where to include documentation on the usage of these helpers. The module docstrings have usage examples but I don't know if there is a better place to include that information.

@nulinspiratie nulinspiratie left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'll skip reviewing the code itself since it's hard to see what changed with the formatting changes also included.
Regarding the UX, we don't need to follow the request verbatim. It's perhaps worth thinking about what the best UX would be.
For example, how to specify the type? Currently a string is expected, but the user is prone to make mistakes for values such as mw_fem_output. Perhaps there's a way to avoid this confusion. Some examples:

machine.add_mw_port(con = 1, slot = 2, port = 1, type="output")
machine.add_mw_output_port(con = 1, slot = 2, port = 1)

Similar arguments could be made for add_channel etc. I'm not sure what the best would be, but it's worth thinking it through once more: What's the most intuitive way for a user to add/remove a qubit/channel/port after a QUAM state has already been created?

@JacobFastQM
JacobFastQM force-pushed the feature/modify-existing-quam branch from 148dc7e to 7228403 Compare June 4, 2026 22:30
@Deepakkhurrana

Copy link
Copy Markdown
Contributor

Before delving deeper into design implementation, I am trying to understand the issue this PR is solving. Lets take an example: when a user needs to add a new qubit to an existing machine that already has calibrated state, they have two paths:

Path A: Edit wiring + re-run build_quam() (without this PR). For example:

machine.wiring["qubits"]["q5"] = {
    "xy": {"opx_output": "#/ports/mw_outputs/con1/1/5"},
    "rr": {...}
}
build_quam(machine)

The advantage of this path is that we dont need a no new API surface and Wiring is always the source of truth
The problems are

  • build_quam() is fully destructive. For ex. add_transmons() resets active_qubit_names = [] and unconditionally overwrites every qubit with a fresh object. It calls machine.save() at the end, stamping over state.json with factory defaults. All calibrated parameters for existing qubits are lost as I understand it.
  • Not a viable option in practice for any machine that has been through calibration

Path B: This PR — add_qubit() helper

Advantages:

  • Surgically adds only the new qubit without touching existing ones
  • Calibrated state for all existing qubits is fully preserved
  • Solves the real problem today without modifying the core builder

Cons:

  • Duplicates logic from build_quam() — every time the builder evolves, modify_quam.py needs to stay in sync
  • Dual-write: has to update both machine.qubits (object tree) and machine.wiring (dict) simultaneously, a mid-way exception leaves the machine in a corrupted, unrecoverable state

Path A fails because build_quam() is not idempotent (as I understand now). Path B works around that by bypassing build_quam() entirely. The real fix would be making build_quam() idempotent, skip qubits that already exist, at which point Path A works correctly and Path B becomes unnecessary. I think this PR is a pragmatic short-term solution worth merging to unblock users, but worth tracking against a future idempotent builder. Thoughts?

@JacobFastQM

Copy link
Copy Markdown
Contributor Author

@Deepakkhurrana You have it basically correct. One slight note: the new helpers don't fully duplicate the build_quam process but reuse most of its atomic adders (i.e. add_default_transmon_pulses) to actually modify the machine, so changes to the core components of build_quam would still apply to the modification helpers. However, you are correct that they are still somewhat decoupled which could result in sync issues.

In general, my proposal in the PR is short term anyway because the helpers are only applicable to the superconducting side of the repo. Currently, the machines for quantum_dots largely have a different architecture and there is no unified Quam base class which would hold the implementation (or at least declaration) of these helpers. To me that would occur in a more involved refactor of the entire repo or possibly during the qua-libs breakup when we try moving the relevant builders to their respective modality repos.

I'm open to changing the structure of this PR to make build_quam idempotent. Do you think that is a better solution for this specific PR or should I stick with the current implementation?

@nulinspiratie Also curious to hear your take on this design choice.

@Deepakkhurrana Deepakkhurrana left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The overall direction makes sense as a pragmatic short-term helper, but I think we should fix a few state-safety issues before merging.

Couple of general comments:

  1. I agree with @nulinspiratie point about naming the helpers.
  2. The new add_qubit() / add_channel() helpers currently only materialize #/ports/... references. They do not materialize #/octaves/... or #/mixers/... dependencies, even though the underlying transmon adders support wiring with frequency_converter_up/down. So Octave/external-mixer wiring is either unsupported right now or can produce channels with unresolved frequency-converter references. The helpers should either support those dependencies or explicitly reject/document that only direct port wiring is supported.

Comment thread quam_builder/builder/superconducting/modify_quam.py
Comment thread quam_builder/builder/superconducting/modify_quam.py
Comment thread quam_builder/builder/superconducting/modify_quam.py
Comment thread quam_builder/builder/qop_connectivity/modify_ports.py
@JacobFastQM
JacobFastQM force-pushed the feature/modify-existing-quam branch from 8d276f9 to f8e3428 Compare June 11, 2026 20:23
@JacobFastQM

Copy link
Copy Markdown
Contributor Author

@Deepakkhurrana @nulinspiratie
Addressed the review feedback, could you take another look?

Issue My attempt to address
Generic port_type strings were easy to get wrong Explicit port helpers (add_mw_port, add_analog_port, add_digital_port) with con/slot/port and required type="input" or type="output"; kept add_port/remove_port
Helpers could mutate the machine before failing validation Validate wiring and port mappings first, stop on failure
add_channel re-seeded all default pulses and overwrote existing ones Only fill missing ops for the line being added
remove_qubit didn't check qubit pairs Errors if the qubit is referenced by a pair, no cascade removal for now
Octave/mixer wiring refs weren't supported Create missing #/ports/..., #/octaves/..., and #/mixers/... refs
_sanitize_fem_mw_kwargs see comment above

We can open an issue to make build_quam idempotent as a long term solution.

@Deepakkhurrana Deepakkhurrana left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for addressing the previous comments, I have a comment on new changes.

Comment thread quam_builder/builder/superconducting/add_default_pulses.py Outdated

@Deepakkhurrana Deepakkhurrana left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@JacobFastQM
JacobFastQM merged commit a21df9b into main Jun 16, 2026
5 checks passed
@JacobFastQM
JacobFastQM deleted the feature/modify-existing-quam branch June 16, 2026 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.

3 participants