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

Add PhaseRootNMask to QInterface, implement for QEngineCPU #1009

Merged
merged 3 commits into from
Jun 17, 2024

Conversation

jpacold
Copy link
Contributor

@jpacold jpacold commented Jun 9, 2024

Draft PR to start addressing #876 -- I would appreciate feedback about whether I'm on the right track.

Reading the issue description, I think the intended behavior of PhaseRootNMask(N, m) is:

$$ \left|x \right> \rightarrow \exp( qi\pi /2^{N-1} ) \left|x \right> $$

where q = popcount(x & m) or equivalently q = popcount(x & m) % (1 << N) . So far I have only added implementations to QInterface and QEngineCPU.

One thing I'm confused about: the base implementation of QInterface::PhaseRootN calculates a phase factor of pow(-ONE_CMPLX, (real1)(ONE_R1 / pow2Ocl(n - 1U))). This gives $\exp(-i\pi / 2^{n-1})$ where I would expect $\exp(+i\pi / 2^{n-1})$. As a result the test that I added currently doesn't pass.

@WrathfulSpatula
Copy link
Collaborator

WrathfulSpatula commented Jun 10, 2024

Thanks for the PR! You are on the right track. Yes, this is equivalent to applying a phase factor of $(-1)^{1/2^{N-1}}$ on every bit in |1> state.

I'll clarify that the reason for the sign convention is so the form of the phase factor will be $(-1)^{1/2^{N-1}}$. So, the point is that the base is $-1$, and this is a root rather than a power. Your convention would make more sense for "PhasePowerN()," as a method name, if you see my point.

Let me know if I can help with the OpenCL side of the implementation or any further questions!

@jpacold
Copy link
Contributor Author

jpacold commented Jun 11, 2024

Thanks, I changed the sign convention. I will definitely need help with OpenCL since this is the first time I've seen it, but I think the earliest I can revisit this will be Thursday or Friday night.

@WrathfulSpatula
Copy link
Collaborator

@jpacold, please comment directly on #876 so I can assign you and close the issue before the end of the event. You have a CPU-based implementation, at least, and you deserve the bounty.

@jpacold
Copy link
Contributor Author

jpacold commented Jun 12, 2024

Thanks, much appreciated. 😃 Should I open another issue for the OpenCL implementation or just open another PR when I have something?

@jpacold jpacold marked this pull request as ready for review June 12, 2024 21:10
@jpacold
Copy link
Contributor Author

jpacold commented Jun 17, 2024

I finally got a chance to get back to this and did the following:

  • Added an OpenCL implementation of PhaseRootNMask. There is more repetition than I would like of the code in QEngineOCL::BitMask. Basically this is because I wanted to pass an additional argument nPhases = pow(2, N) to the OpenCL function. I guess I could add an optional argument to BitMask, but that will make the logic in BitMask more complicated, so I'm not sure what the right trade-off is here.
  • Added another case to the unit test, because I didn't have coverage for the special case where the mask is a power of 2.

Copy link
Collaborator

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

@jpacold, wow, this is amazing! I was honestly just returning to this PR to say that you did 90% or more of the "brain work" just by producing the CPU-based implementation, but it's amazing that you took a shot at the OpenCL, and I'm very impressed that you understand Qrack's relatively complicated inheritance and style guide!

Pardon me for scooping this up to take it "over the finish line" from here, but you get the idea, and you've earned the bounty. I can easily complete this from here, much more so for your attempt at an OpenCL kernel. I will say for your reference, to give this closure, at least on previous-generation hardware and drivers (on which we put some support priority) if not through to today, you should know there's a very high overhead premium on multiple argument buffers being passed to the OpenCL kernels. Your implementation is nearly perfect, to Qrack standards, except, since the phase angle can be reconstructed this way:

const real1_f radians[1] = { -PI_R1 / pow2Ocl(n - 1U) };

it's actually still very preferable to reconstruct radians from the n argument on device-side, in OpenCL, from another bitCapIntOcl argument in the single buffer of all bitCapIntOcl arguments. The reason it's preferable is because electronic bus transfer to-and-from OpenCL devices will end up "coalesced" if done according to ideal best-practice, such that two buffers would require two separate writes on the bus, while one buffer will likely be passed in the full width of a single write operation on the bus.

You've done excellently, though! I can finish this in less than an hour, maybe less than 30 minutes, and it would likely take significantly more work and consideration from you. I want to roll a release today, for other reasons, so I'm accepting your PR, to do the bit of work left to complete it, myself.

Thank you so much! Please, let's just make sure you get your bounty! Come talk with me, if you have any issue claiming the incentive for your work!

@WrathfulSpatula
Copy link
Collaborator

By the way, don't freak about the test failure: it's a very rare stochastic failure that has nothing to do with your work, and I've seen it before. Let's merge! Thanks again!

@WrathfulSpatula WrathfulSpatula merged commit 90af46e into unitaryfund:main Jun 17, 2024
2 of 3 checks passed
@jpacold
Copy link
Contributor Author

jpacold commented Jun 17, 2024

You're welcome and thanks for taking over from here. After seeing all the Python/Rust projects out there it was great to find a mainly C++ issue to work on!

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