Skip to content

fix: 🧑‍🔬 use std::optional for only_rotate_len_k argument - #210

Merged
robertodr merged 12 commits into
mainfrom
copilot/switch-to-std-optional-int
Aug 10, 2026
Merged

fix: 🧑‍🔬 use std::optional for only_rotate_len_k argument#210
robertodr merged 12 commits into
mainfrom
copilot/switch-to-std-optional-int

Conversation

Copilot AI commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Fix #6

🤖 AI text below 🤖

only_rotate_len_k used an integer sentinel in C++ despite being optional in Python. This made None cross the binding boundary as 0 instead of preserving optional semantics.

Changes

  • C++ API

    • Use std::optional<int> throughout propagation.
    • Treat std::nullopt as unrestricted gate application.
  • Python bindings

    • Default nanobind arguments to std::nullopt.
    • Pass Python None through without conversion.
  • Coverage and documentation

    • Cover explicit None for both propagation modes.
    • Document the unrestricted default.
std::optional<int> only_rotate_len_k = std::nullopt;

Copilot AI and others added 3 commits August 5, 2026 18:36
Assisted-by: GitHubCopilot:gpt-5.4

Co-authored-by: robertodr <3708689+robertodr@users.noreply.github.com>
Assisted-by: GitHubCopilot:gpt-5.4

Co-authored-by: robertodr <3708689+robertodr@users.noreply.github.com>
Assisted-by: GitHubCopilot:gpt-5.4

Co-authored-by: robertodr <3708689+robertodr@users.noreply.github.com>
Comment thread README.md Outdated
@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown

Docs preview: https://pr-210.monoprop-docs.pages.dev

Co-authored-by: Roberto Di Remigio Eikås <robertodr@users.noreply.github.com>
Signed-off-by: Roberto Di Remigio Eikås <robertodr@users.noreply.github.com>
@github-actions github-actions Bot added documentation Improvements or additions to documentation python cpp labels Aug 6, 2026
@robertodr
robertodr marked this pull request as ready for review August 6, 2026 10:03
@robertodr
robertodr requested a lite review from Copilot and removed request for adamglos92 and fpietra August 6, 2026 10:03
@codecov

codecov Bot commented Aug 6, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.70%. Comparing base (680d956) to head (eddb8d9).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #210      +/-   ##
==========================================
+ Coverage   97.58%   97.70%   +0.12%     
==========================================
  Files          14       14              
  Lines         745      742       -3     
  Branches       99       98       -1     
==========================================
- Hits          727      725       -2     
+ Misses         13       12       -1     
  Partials        5        5              
Flag Coverage Δ
cpp 97.70% <100.00%> (+0.12%) ⬆️

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

Copilot AI 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.

Pull request overview

This PR aligns the only_rotate_len_k gate-application length cap semantics across Python ↔ nanobind ↔ C++ by replacing the C++ int sentinel with std::optional<int> and treating std::nullopt as “no cap”.

Changes:

  • Update the C++ propagation API and internal evolution codepaths to use std::optional<int> for only_rotate_len_k.
  • Adjust nanobind defaults to std::nullopt so Python None can cross the binding boundary without sentinel conversion.
  • Add/adjust documentation and tests around None/uncapped behavior.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
tests/test_only_rotate_k.py Adds None to the validation parametrizations for only_rotate_len_k.
src/monoprop/monomial_propagator.py Switches validation helper to preserve None (no None → 0 correction) and passes optional through to the engine.
src/monoprop/bindings/binder.h Changes nanobind defaults for only_rotate_len_k to std::nullopt.
docs/content/docs/features/cutoff.mdx Documents only_rotate_len_k and clarifies that omitting / passing None is unrestricted.
cpp/monoprop/detail/monomial_propagator/MonomialPropagatorImpl.h Threads std::optional<int> through build/propagate paths and gate loop plumbing.
cpp/monoprop/detail/evolution/layer_build/Scan.h Updates scan gating logic to accept std::optional<int> and interpret nullopt as uncapped.
cpp/monoprop/detail/evolution/layer_build/FusedApply.h Updates comments to reflect “no length cap” rather than k==0 sentinel behavior.
cpp/monoprop/detail/evolution/layer_build/Engine.h Updates fused scaling eligibility from k==0 to “no length cap” (nullopt).
cpp/include/monoprop/MonomialPropagator.h Changes public C++ API defaults to std::nullopt and documents uncapped semantics.
AGENTS.md Updates architecture notes to reflect std::optional<int> / std::nullopt semantics.
Suppressed comments (2)

src/monoprop/monomial_propagator.py:186

  • 🤖 AI text below 🤖
    The docstring says the upper-bound check only applies when the propagator “knows its qubit count (i.e. on a PauliPropagator)”, but the implementation always enforces only_rotate_len_k <= 2 * self._system_size for all propagators. Please update the docstring to match the actual validation rule to avoid misleading users.
        Must be positive, and at most ``2 * num_qubits`` when the propagator knows its qubit count
        (i.e. on a [PauliPropagator][monoprop.pauli_propagator.PauliPropagator]).

cpp/monoprop/detail/monomial_propagator/MonomialPropagatorImpl.h:693

  • 🤖 AI text below 🤖
    Same as build_graph: propagate accepts std::optional<int> only_rotate_len_k but doesn’t validate it. A negative value will be cast to size_t and behave like “no cap”, which is likely unintended and differs from the Python validation.
auto MonomialPropagator<NumModes>::propagate(const std::vector<VecZ> &majoranas,
                                             const VecZ &parameter_mapping,
                                             const VecD &gen_coeffs,
                                             const VecD &parameters,
                                             std::optional<int> only_rotate_len_k) -> void {

Comment thread cpp/monoprop/detail/monomial_propagator/MonomialPropagatorImpl.h Outdated
Comment thread tests/test_only_rotate_k.py
Comment thread cpp/monoprop/detail/evolution/layer_build/Scan.h
@robertodr robertodr changed the title fix: 🐛 use optional rotation length cutoff fix: :doctor: use optional rotation length cutoff Aug 6, 2026
@robertodr robertodr changed the title fix: :doctor: use optional rotation length cutoff fix: 🧑‍🔬 use optional rotation length cutoff Aug 6, 2026
@robertodr
robertodr requested a review from ludmilaasb August 6, 2026 13:32
@robertodr robertodr changed the title fix: 🧑‍🔬 use optional rotation length cutoff fix: 🧑‍🔬 use std::optional for only_rotate_len_k argument Aug 6, 2026

@adamglos92 adamglos92 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

only a refactoring nitpick (too much C++ for me to review)

Comment thread src/monoprop/monomial_propagator.py Outdated
Comment thread tests/test_only_rotate_k.py
@sonarqubecloud

Copy link
Copy Markdown

@ludmilaasb ludmilaasb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@robertodr
robertodr merged commit 16c3255 into main Aug 10, 2026
34 of 35 checks passed
@robertodr
robertodr deleted the copilot/switch-to-std-optional-int branch August 10, 2026 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cpp documentation Improvements or additions to documentation python test-in-draft Run CI even in Draft mode

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Use a more explicit default for only_rotate_len_k

5 participants