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

Minor bug fixes. #18

Merged
merged 2 commits into from May 21, 2022
Merged

Minor bug fixes. #18

merged 2 commits into from May 21, 2022

Conversation

Zshan0
Copy link
Contributor

@Zshan0 Zshan0 commented May 21, 2022

There were a few minor bugs that went unnoticed, a few more of them persist that require discussion that can be continued on this thread before it is merged with the main branch.

@WrathfulSpatula
Copy link
Contributor

@Zshan0 This is great! My only particular point to add is that dump_callback() and dump_ids_callback() should not have the self parameter, because they are specifically C callbacks, called from the Python code by the underlying C++ Qrack library itself, believe it or not. If you could revert just those two self parameters, we'll get this right in!

@Zshan0
Copy link
Contributor Author

Zshan0 commented May 21, 2022

I'm suggesting a few changes that could be discussed and bundled together in this PR itself.

  1. Wildcard imports are not recommended from libraries, this is a minor fix that could be issued as a good first issue.
  2. I'm not fully sure, but I believe that permutation_expectation here must return the value result. The same applies for joint_ensemble_probability. If that is the correct behavior, I'll just add it to this PR.

Another suggestion that I would make is that the upcoming unitary hack is a good opportunity to add tests/CI for pyqrack :D

@WrathfulSpatula
Copy link
Contributor

@Zshan0 Correct, permutation_expectation() and joint_ensemble_probability() should return their result, after the shared C library interface is polled for exception. As for the ctypes wildcard import, you can go ahead and import the explicit ctypes native type classes that we use, if you like.

@Zshan0
Copy link
Contributor Author

Zshan0 commented May 21, 2022

I can do import ctypes and make changes from c_ulonglong to ctypes.c_ulonlong rather than importing all the required classes, whatever you think would be better :)

@WrathfulSpatula
Copy link
Contributor

Honestly, you're probably more aware of Python standards than I am, so I leave the choice for ctypes to your best judgement! (I'm basically a C++ developer providing an interface, in this case.)

Copy link
Contributor

@WrathfulSpatula WrathfulSpatula left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks again! Feel free to merge, whenever you're ready! (I'll likely iterate a release with your changes, sometime today, because that's the precedent PyQrack maintains for support!)

@WrathfulSpatula WrathfulSpatula merged commit 0736217 into unitaryfund:main May 21, 2022
@Zshan0 Zshan0 deleted the typos branch May 21, 2022 09:41
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.

None yet

2 participants