tket2 icon indicating copy to clipboard operation
tket2 copied to clipboard

feat!: Replace Subcircuit // take 4

Open lmondada opened this issue 1 month ago • 5 comments

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 avatar Nov 23 '25 20:11 lmondada

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 avatar Nov 23 '25 20:11 lmondada

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

hugrbot avatar Nov 23 '25 20:11 hugrbot

Codecov Report

:x: Patch coverage is 73.54618% with 232 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 78.63%. Comparing base (0f1d700) to head (7ada59e). :warning: 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 :warning:
tket/src/resource.rs 69.35% 18 Missing and 1 partial :warning:
tket/src/rewrite/strategy.rs 5.55% 17 Missing :warning:
tket/src/resource/scope.rs 85.71% 5 Missing and 2 partials :warning:
tket/src/portmatching/matcher.rs 33.33% 4 Missing :warning:
tket/src/resource/types.rs 0.00% 2 Missing :warning:
tket/src/rewrite.rs 83.33% 1 Missing and 1 partial :warning:
tket/src/rewrite/trace.rs 0.00% 1 Missing :warning:
tket/src/subcircuit/interval.rs 88.88% 0 Missing and 1 partial :warning:
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%) :arrow_down:

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

: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.

codecov[bot] avatar Nov 23 '25 20:11 codecov[bot]

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 avatar Nov 23 '25 21:11 lmondada

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.

acl-cqc avatar Nov 28 '25 15:11 acl-cqc

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 avatar Dec 03 '25 13:12 lmondada