Skip to content

Conversation

@lmondada
Copy link
Contributor

@lmondada lmondada commented Nov 23, 2025

Sorry for the delay! Mostly copying from #1223:

Hey Alan,

As promised, here is the subcircuit PR, rewritten to use CopyableExpr. This way subcircuits can be used to represent any valid SiblingSubgraph (indeed, a superset of SiblingSubgraph, as certain non-convex subgraphs are expressible too).

Note that I have replaced many of the "old" uses of Subcircuit with straight-forward SiblingSubgraph. The extra abstraction layer was pretty useless. It might sense to migrate those (or new versions of them) to the new Subcircuit at some point, but that is orthogonal to this PR.

The convexity check will come in a separate PR.

BREAKING CHANGE: New API for Subcircuit, see docs. The Rewrite trait now takes a generic node argument.

@lmondada lmondada requested a review from a team as a code owner November 23, 2025 20:29
@lmondada lmondada requested a review from doug-q November 23, 2025 20:29
@lmondada
Copy link
Contributor Author

I can't change the assigned reviewer. @doug-q feel free to have a look at this yourself or pass it on to @acl-cqc

@lmondada lmondada force-pushed the lm/subcirc-pr branch 2 times, most recently from e133a67 to 8fb512c Compare November 23, 2025 20:32
@hugrbot
Copy link
Collaborator

hugrbot commented Nov 23, 2025

This PR contains breaking changes to the public Rust API.

cargo-semver-checks summary
    Building tket v0.16.0 (current)
     Built [  43.355s] (current)
   Parsing tket v0.16.0 (current)
    Parsed [   0.099s] (current)
  Building tket v0.16.0 (baseline)
     Built [  41.125s] (baseline)
   Parsing tket v0.16.0 (baseline)
    Parsed [   0.083s] (baseline)
  Checking tket v0.16.0 -> v0.16.0 (assume minor change)
   Checked [   0.079s] 159 checks: 156 pass, 3 fail, 0 warn, 41 skip

--- failure inherent_method_missing: pub method removed or renamed ---

Description:
A publicly-visible method or associated fn is no longer available under its prior name. It may have been renamed or removed entirely.
      ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
     impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.45.0/src/lints/inherent_method_missing.ron

Failed in:
PatternMatch::subcircuit, previously in file /home/runner/work/tket2/tket2/BASELINE_BRANCH/tket/src/portmatching/matcher.rs:99
PatternMatch::subcircuit, previously in file /home/runner/work/tket2/tket2/BASELINE_BRANCH/tket/src/portmatching/matcher.rs:99
CircuitRewrite::subcircuit, previously in file /home/runner/work/tket2/tket2/BASELINE_BRANCH/tket/src/rewrite.rs:117
ResourceScope::get_port, previously in file /home/runner/work/tket2/tket2/BASELINE_BRANCH/tket/src/resource/scope.rs:185
ResourceScope::get_copyable_wires, previously in file /home/runner/work/tket2/tket2/BASELINE_BRANCH/tket/src/resource/scope.rs:249
Subcircuit::try_from_nodes, previously in file /home/runner/work/tket2/tket2/BASELINE_BRANCH/tket/src/rewrite.rs:38
Subcircuit::signature, previously in file /home/runner/work/tket2/tket2/BASELINE_BRANCH/tket/src/rewrite.rs:57

--- failure method_parameter_count_changed: pub method parameter count changed ---

Description:
A publicly-visible method now takes a different number of parameters.
      ref: https://doc.rust-lang.org/cargo/reference/semver.html#fn-change-arity
     impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.45.0/src/lints/method_parameter_count_changed.ron

Failed in:
tket::rewrite::Subcircuit::nodes now takes 2 parameters instead of 1, in /home/runner/work/tket2/tket2/PR_BRANCH/tket/src/subcircuit.rs:409
tket::rewrite::Subcircuit::node_count now takes 2 parameters instead of 1, in /home/runner/work/tket2/tket2/PR_BRANCH/tket/src/subcircuit.rs:461

--- failure struct_missing: pub struct removed or renamed ---

Description:
A publicly-visible struct cannot be imported by its prior path. A `pub use` may have been removed, or the struct itself may have been renamed or removed entirely.
      ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
     impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.45.0/src/lints/struct_missing.ron

Failed in:
struct tket::resource::ResourceScopeConfig, previously in file /home/runner/work/tket2/tket2/BASELINE_BRANCH/tket/src/resource/scope.rs:63

   Summary semver requires new major version: 3 major and 0 minor checks failed
  Finished [  86.482s] tket
  Building tket-qsystem v0.22.0 (current)
     Built [  42.788s] (current)
   Parsing tket-qsystem v0.22.0 (current)
    Parsed [   0.028s] (current)
  Building tket-qsystem v0.22.0 (baseline)
     Built [  42.359s] (baseline)
   Parsing tket-qsystem v0.22.0 (baseline)
    Parsed [   0.028s] (baseline)
  Checking tket-qsystem v0.22.0 -> v0.22.0 (assume minor change)
   Checked [   0.044s] 159 checks: 159 pass, 41 skip
   Summary no semver update required
  Finished [  87.366s] tket-qsystem

@codecov
Copy link

codecov bot commented Nov 23, 2025

Codecov Report

❌ Patch coverage is 73.54618% with 232 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.63%. Comparing base (0f1d700) to head (7ada59e).
⚠️ Report is 10 commits behind head on main.

Files with missing lines Patch % Lines
tket/src/subcircuit.rs 74.78% 165 Missing and 14 partials ⚠️
tket/src/resource.rs 69.35% 18 Missing and 1 partial ⚠️
tket/src/rewrite/strategy.rs 5.55% 17 Missing ⚠️
tket/src/resource/scope.rs 85.71% 5 Missing and 2 partials ⚠️
tket/src/portmatching/matcher.rs 33.33% 4 Missing ⚠️
tket/src/resource/types.rs 0.00% 2 Missing ⚠️
tket/src/rewrite.rs 83.33% 1 Missing and 1 partial ⚠️
tket/src/rewrite/trace.rs 0.00% 1 Missing ⚠️
tket/src/subcircuit/interval.rs 88.88% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1273      +/-   ##
==========================================
- Coverage   79.46%   78.63%   -0.84%     
==========================================
  Files         160      161       +1     
  Lines       20378    21144     +766     
  Branches    19446    20212     +766     
==========================================
+ Hits        16194    16626     +432     
- Misses       3201     3528     +327     
- Partials      983      990       +7     
Flag Coverage Δ
python 92.65% <ø> (ø)
qis-compiler 100.00% <ø> (ø)
rust 77.96% <73.54%> (-0.85%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@lmondada
Copy link
Contributor Author

The reason for the reduced coverage in tket/src/rewrite/strategy.rs is the temporarily ignored test. This will be reactivated with the next PR.

@lmondada lmondada force-pushed the lm/subcirc-pr branch 4 times, most recently from 96ddded to 659ad39 Compare November 23, 2025 21:25
@acl-cqc
Copy link
Contributor

acl-cqc commented Nov 28, 2025

FYI I've broken this up into #1288 (which I think can go straight in, I've made a couple of tiny modifications), #1289 (the bulk, needs reviewing), and #1290 (questions/concerns that much of the conversion-folding code should not be needed). Diffing the three together against this reveals that they are nearly identical, the only significant change being that I've skipped the parametrization of Rewriter since that wasn't used anywhere.

Will update more when I've looked at those more.

@lmondada
Copy link
Contributor Author

lmondada commented Dec 3, 2025

I've skipped the parametrization of Rewriter since that wasn't used anywhere

Thank you so much Alan! The parametrization will be required for the next PR I'll open, but I guess that change can wait until then :)

Thanks again!

@lmondada lmondada closed this Dec 3, 2025
@lmondada lmondada deleted the lm/subcirc-pr branch December 3, 2025 13:38
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