Skip to content

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

Open
robertodr with Copilot wants to merge 8 commits into
mainfrom
copilot/switch-to-std-optional-int
Open

fix: 🧑‍🔬 use std::optional for only_rotate_len_k argument#210
robertodr with Copilot wants to merge 8 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.71%. Comparing base (5786343) to head (1f50cce).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #210      +/-   ##
==========================================
+ Coverage   97.58%   97.71%   +0.12%     
==========================================
  Files          14       14              
  Lines         745      743       -2     
  Branches       99       98       -1     
==========================================
- Hits          727      726       -1     
+ Misses         13       12       -1     
  Partials        5        5              
Flag Coverage Δ
cpp 97.71% <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
const size_t i = w.base + tz;
const size_t mono_pop = op.store->popcount(i);
if (mono_pop > static_cast<size_t>(only_rotate_len_k)) {
if (mono_pop > static_cast<size_t>(*only_rotate_len_k)) {

@ludmilaasb ludmilaasb Aug 6, 2026

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.

What happens when you try to cast to size_t but the value is a nullopt?

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.

in the else branch only_rotate_len_k is guaranteed to have a value, since const bool word_aligned_cos = !only_rotate_len_k; is false in that case.

@sonarqubecloud

sonarqubecloud Bot commented Aug 6, 2026

Copy link
Copy Markdown

@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
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

4 participants