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

noncliff: enhanced error message for inapplicable Project! of GeneralizedStabilizer #416

Merged
merged 12 commits into from
Nov 8, 2024

Conversation

Fe-r-oz
Copy link
Contributor

@Fe-r-oz Fe-r-oz commented Nov 3, 2024

I have incorporated the relevant details from the paper to create an informed error message. This PR is inspired from this comment: #355 (comment)

I think we only need one proj, so removed _proj₋ or _proj₊ in favor of proj.

  • The code is properly formatted and commented.
  • Substantial new functionality is documented within the docs.
  • All new functionality is tested.
  • All of the automated tests on github pass.
  • We recently started enforcing formatting checks. If formatting issues are reported in the new code you have written, please correct them. There will be plenty of old code that is flagged as we are slowly transitioning to enforced formatting. Please do not worry about or address older formatting issues -- keep your PR just focused on your planned contribution.

@Fe-r-oz Fe-r-oz force-pushed the fa/errormsgproj branch 2 times, most recently from a87be4c to 393e59a Compare November 3, 2024 07:15
@Fe-r-oz Fe-r-oz marked this pull request as draft November 3, 2024 09:25
@Fe-r-oz Fe-r-oz marked this pull request as ready for review November 3, 2024 15:30
@Fe-r-oz
Copy link
Contributor Author

Fe-r-oz commented Nov 3, 2024

The PR is ready for review. Please run it on the console to check if any further formatting changes are required.

Thank you!

Copy link
Member

@Krastanov Krastanov left a comment

Choose a reason for hiding this comment

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

looks good! I will do some minor rewording on the next round of review, but first check out below for how this can be packaged a bit more neatly into Julia's error handling mechanism.

@@ -172,15 +172,41 @@ function _allthreesumtozero(a,b,c)
true
end

function project!(sm::GeneralizedStabilizer, p::PauliOperator)

project!(::GeneralizedStabilizer, ::PauliOperator) =
Copy link
Member

Choose a reason for hiding this comment

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

this can be done a bit more neatly using MethodError and a registered error hint.

E.g. throw throw(MethodError(f, (sm,p))) where sm and p are the arguments to this function.

Then use register_error_hint to register the additional explanation message: https://docs.julialang.org/en/v1.12-dev/base/base/#Base.Experimental.register_error_hint

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks so much for the suggestion!

@Krastanov
Copy link
Member

marking as draft to make it easer for me to track what needs review

@Krastanov Krastanov marked this pull request as draft November 5, 2024 15:24
@Fe-r-oz Fe-r-oz marked this pull request as ready for review November 5, 2024 23:59
@Fe-r-oz
Copy link
Contributor Author

Fe-r-oz commented Nov 6, 2024

Thank you! I have incorporated your comments and checked the output on console.

Thank you for rewording as you see fit!

Edit:

Fixed some minor doctests test errors in recent commits

@Krastanov Krastanov merged commit f90a4ed into QuantumSavory:nonclif Nov 8, 2024
8 of 12 checks passed
@Fe-r-oz Fe-r-oz deleted the fa/errormsgproj branch November 8, 2024 22:38
@Fe-r-oz
Copy link
Contributor Author

Fe-r-oz commented Nov 9, 2024

Thank you for polishing the PR!

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.

2 participants