-
Notifications
You must be signed in to change notification settings - Fork 180
Add repeated operation #153
Add repeated operation #153
Conversation
|
Hi @christopherkang, thanks for opening this PR. I would expect a different signature from |
|
@msoeken - good suggestion! What's the optimal sorting algorithm to be used? |
|
@christopherkang It may be too time consuming to find the optimal decomposition into transpositions. But one simple technique is to apply transpositions to always move the largest element that is not yet in its position, until the identity is reached. Then you need at most |
|
@msoeken is this achievable without mutating an array? It would be preferable to have this op as Adj+Ctl. |
|
@christopherkang: One trick is to isolate mutability within a function, then call to that function from within an operation with |
|
Hi @christopherkang, thanks for the additions. I have worked on a code for permutation synthesis some weeks ago, but it hasn't made it yet to the libraries. There is no need to have two functions that do the same thing, so I'd like to share my code with you as it may be helpful for your implementation. |
|
Also note that functions such as |
|
@msoeken I updated my code with your suggestions. I've kept claims in for now - after Quantum.Logical gets pushed, I'll update the code with the new methods |
cgranade
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took a quick look over, and this is looking really good, thanks for the contribution! The main thing I'd like to iterate on would be the names for ApplySeriesOfOps and ApplyOpRepeatedlyOver. For the latter, I think ApplyToEachSubregister would fit well with the established naming conventions around ApplyToEach, ApplyToEachIndex, and ApplyToSubregister. For the former, I'd like to find a similar name that emphasizes the commonality with other ApplyToEach operations; my first inclination is something like ApplyEachToEachSubregister, but that isn't as readable as I'd like yet, sorry.
Anyway, I think this is a great start; once #152 comes in, and you've refactored to use those new functions, I this should be ready for a more detailed review. Thank you so much!
Co-Authored-By: Chris Granade <cgranade@gmail.com>
msoeken
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution! It's well organised but I have some comments for clarity, since many functions will go into the public API. This is in addition to @cgranade's comments. In particular we should wait for the other PR to reuse some of the helper functions and make this PR more concise.
|
Thanks, @msoeken and @cgranade - I appreciate the help! I've updated the code with the PR'd functions, added examples to the docs. Here's our remaining action items (alongside code review)
Let me know if I missed anything! |
msoeken
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code is really taking a great shape! There are some more comments to make it more concise.
|
Thanks, @msoeken! I appreciate it 😃 I was ideating on potential names - what about ApplyEachToSubarrays (for different ops) and ApplyToSubarrays (for the same op). Open to suggestions! |
|
@christopherkang Thanks for the last changes, all my comments have been addressed. I cannot comment much on the naming of the functions, @cgranade, could you please check whether the names of the new functions are okay. |
cgranade
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution, and for your patience! I think this looks really good, other than that I'd suggest renaming your type conversion function, and that I'd suggest either dropping Op or expanding Op to Operation. Marking as approved, as I'm happy to pick that up after merging that in if you'd like. Thank you so much for your contribution, excited to have this in!
|
Thank you both! @cgranade I renamed the Array function, but wasn't sure what you meant by type conversion function. You're welcome to merge changes, once checks pass! I can also make those edits, I'll just need some clarification. Whatever's easier for you! 😄 |
|
My apologies for missing the GitHub notification about your comment, @christopherkang! Merging in now, thank you so much for your contribution. 💕 |
|
No worries @cgranade - are you aware of any areas that could be refactored with these operations? If so, I'll start working on a new PR |
|
I think your most recent round addressed everything, @christopherkang, great to have your contribution on board for the next release! My apologies again for missing the notification. |
* Drafted new apply ops * Fixed docs * Fixed minor bugs * Added Permutation function + helper Arrays + Claim functions * Added Adj + Ctl and set csproj back to generating docs * Added tests * Fixed test errors * Updated code from comments; moved PermuteQubits to CommonGates * Fixed minor bugs * Apply suggestions from code review Co-Authored-By: Chris Granade <cgranade@gmail.com> * Added some fixes from changes * Added most recommendations from Mathias * Added example to ApplySeriesOfOps * Added new examples * Added PermuteQubits example * Changed Swap Order to be appending to an array, added test for it * Updated ApplyOpRepeatedlyOver Docs * Added Mathias' comments * Reverted csproj file * Renamed TupleArrayAsNestedArray
|
Awesome! Also, @cgranade, I mean do you know of any code in the wider Q# codebase that could make use of this operation? |
Draft! Adding the following ops:
Would like some input on a few things:
To-do: