Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't unwrap when not synthesizing witness. #275

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

porcuquine
Copy link
Contributor

This PR is intended to fix argumentcomputer/bellpepper#90.

I've tested it against an arecibo branch @tchataigner supplied, and which reproduces the issue.

However, it would be best to have a local test demonstrating both the original problem and that this fixes it.

I believe @tchataigner is going to port TestShapeCS from arecibo to bellpepper, after which the natural test of shape-only synthesis can be added to neptune.

Ideally, this PR can then be rebased on a commit demonstrating the failing test. Then we can merge the result with reasonable confidence.

Copy link

@wwared wwared left a comment

Choose a reason for hiding this comment

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

This LGTM, but holding off on approving until we have a specific test exercising this codepath (or we decide on merging this before the TestShapeCS bellpepper PR, whichever comes first). Thanks for the simple fix! 🙏

Signed-off-by: Thomas Chataigner <[email protected]>
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.

Repercussion of latest Num addition changes
3 participants