Skip to content

Remove usage of sbp_mult from docstrings - #733

Open
Shanwis wants to merge 3 commits into
eclipse-qrisp:mainfrom
Shanwis:fix/remove-sbp-mult-usage
Open

Remove usage of sbp_mult from docstrings#733
Shanwis wants to merge 3 commits into
eclipse-qrisp:mainfrom
Shanwis:fix/remove-sbp-mult-usage

Conversation

@Shanwis

@Shanwis Shanwis commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Description

Closes #509
Closes #736

Removes references to the deprecated sbp_mult function from docstrings, replacing them with the standard * operator.

Related Issues

Closes #509
Closes #736

Type of Change

  • Feature (new functionality)
  • Change Request (modification of existing functionality)
  • Bug Fix
  • Refactoring (no behavior change)
  • Performance improvement
  • Documentation
  • CI / Build

Breaking Change?

  • Yes
  • No

What was changed?

  • src/qrisp/core/quantum_session.py: Replaced sbp_mult import and call with qf_0 * qf_1 in the compile docstring example; updated prose and output values
  • src/qrisp/environments/iteration_environment.py: Replaced reference to sbp_mult in the precompilation docstring with generic "multiplication algorithm"

How was it tested?

The updated docstring values were verified by running the example code with uv run.

Test-ID Status
Verified example produces new output values

Screenshots / Output (if applicable)

New compile docstring output:

  • num_qubits(): 45 → 21
  • compile(0).depth(): 497 → 205
  • compile(4).depth(): 258 → 205

Checklist

  • My code follows the project's coding standards
  • I have performed a self-review
  • I have added/updated tests (referencing issue Test-IDs)
  • All tests pass locally and in CI
  • I have updated the documentation
  • I have added a changelog entry to changelog-dev.rst
  • Breaking changes are documented with migration path

Reviewer Notes

N/A

@Shanwis
Shanwis force-pushed the fix/remove-sbp-mult-usage branch from 7d751c4 to 5320270 Compare July 10, 2026 10:38
@Shanwis

Shanwis commented Jul 10, 2026

Copy link
Copy Markdown
Contributor Author

Hi @purva-thakre!

The changelog check is failing with a 403 error when trying to post the acknowledgment comment. This is a known limitation for PRs from forks, the GITHUB_TOKEN has restricted permissions and can't write comments on the upstream repo. The changelog entry itself is correct and was detected successfully; the failure is purely in the comment-posting step, not in the changelog check.

@Shanwis
Shanwis force-pushed the fix/remove-sbp-mult-usage branch from 5320270 to 3316190 Compare July 10, 2026 10:47
@Shanwis

Shanwis commented Jul 10, 2026

Copy link
Copy Markdown
Contributor Author

In public repositories this action does not work in pull_request workflows when triggered by forks. Any attempt will be met with the error, Resource not accessible by integration. This is due to token restrictions put in place by GitHub Actions. Private repositories can be configured to enable workflows from forks to run without restriction. See here for further explanation. Alternatively, use the pull_request_target event to comment on pull requests.

from the https://github.com/peter-evans/create-or-update-comment page

@Shanwis

Shanwis commented Jul 10, 2026

Copy link
Copy Markdown
Contributor Author

I have created issue #736 regarding this.

@purva-thakre

purva-thakre commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Thanks. Yes, I am aware of the issue with the changelog workflow failing to work with fork PRs #714 (comment)

If you have ideas in mind on what a fix could be, feel free to try them out in this PR.

@Shanwis

Shanwis commented Jul 10, 2026

Copy link
Copy Markdown
Contributor Author

I have made the fix for the workflow issue here, I added a check

github.event.pull_request.head.repo.full_name == github.repository

In the comment tasks so that it would not execute the peter-evans/create-or-update-comment action for forked PR's.

Other solutions would be to pull_request_target, but then we would have look into security issues related to using it.

@purva-thakre

Copy link
Copy Markdown
Contributor

Thanks. I will change the workflow such that it does not have to add a comment to the PR. I think this is a better workaround.

@Shanwis

Shanwis commented Jul 10, 2026

Copy link
Copy Markdown
Contributor Author

Yea that is the best I think 😄

Should I remove the changes I have made to the changelog.yml?

@purva-thakre

purva-thakre commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Yes, please undo your change. After that, can you rebase your PR? I added a fix just now in ff426c5

Could you also please remove your entry to changelog-dev to verify the new workflow fails as expected on forked PRs and then add it back to verify it passes? Thanks in advance!

@Shanwis
Shanwis force-pushed the fix/remove-sbp-mult-usage branch from 177eac2 to 4ec1d84 Compare July 10, 2026 13:37
@Shanwis

Shanwis commented Jul 10, 2026

Copy link
Copy Markdown
Contributor Author
image Failed as expected without changelog image with changelog it passes

@purva-thakre

Copy link
Copy Markdown
Contributor

Thank you for helping me debug the new workflow! I will review the changes in your PR by next week.

@purva-thakre purva-thakre added this to the v0.10 milestone Jul 10, 2026
@Shanwis

Shanwis commented Jul 15, 2026

Copy link
Copy Markdown
Contributor Author

Hi @purva-thakre, Please do review the changes when you have the time 🤗

@Shanwis

Shanwis commented Jul 21, 2026

Copy link
Copy Markdown
Contributor Author

Hi @purva-thakre! Please do take a look at this PR when you have the time

:meth:`sbp_mult <qrisp.sbp_mult>` function which heavily profits from more
workspace.
We calculate a product of 2 :ref:`QuantumFloats <QuantumFloat>` which heavily
profits from more workspace.

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 language needs to be altered. With or without workspace, the depth is the same.

Comment on lines +74 to 75
Squaring a :ref:`QuantumFloat` uses a multiplication algorithm
which has a high demand of ancilla qubits.

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.

same here. I do not think high demand of ancilla qubits is true anymore. Check how q_mult works and then alter the language.


.. Add new tutorials above this line


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.

remove this blankline

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.

Remove usage of sbp_mult changelog workflow will always fail for forked PR's

2 participants