This repository was archived by the owner on Jan 12, 2024. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 179
API review May 2022 #584
Merged
Merged
API review May 2022 #584
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
f27b4a1
API review May 2022
msoeken f248c66
Apply suggestions from code review
msoeken eea2d38
Apply suggestions from code review
msoeken 5ad7c39
Update api-design-2022-05.md
msoeken 8428487
Merge branch 'main' into msoeken/api-review-2205
msoeken File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| # Q# API Design Discussions / May 2022 | ||
|
|
||
| Reviewers (in order by username): cgranade | ||
|
|
||
| ## Agenda | ||
|
|
||
| - https://github.com/microsoft/QuantumLibraries/issues/579 | ||
| - https://github.com/microsoft/qsharp-runtime/issues/1005 | ||
| - https://github.com/microsoft/QuantumLibraries/issues/423 | ||
|
|
||
| ## Discussion | ||
|
|
||
| ### Move Exp into libraries instead of intrinsics | ||
|
|
||
| **Proposal**: https://github.com/microsoft/QuantumLibraries/issues/579 | ||
|
|
||
| **Reviews**: | ||
| - **Approved with suggestions.** I think this makes a lot of sense; Exp is extremely useful, but it's much higher-level than any other intrinsic, making it an odd fit for the Microsoft.Quantum.Intrinsics namespace. If I understand correctly, we did so due to the full-state simulator having some nice shortcuts for simulating `Exp`, but I don't think that implementation detail justifies including `Exp` in the basic set of intrinsics. | ||
| I would suggest, however, that if we move `Exp` and make a breaking change that way, we also take the opportunity to give it a more clear name that reflects its action a bit better. In particular, `Exp` simulates evolution under a Pauli Hamiltonian such that `Microsoft.Quantum.Simulation.EvolveUnderPauli` may make sense; alternatively, it can be thought of as applying a joint rotation, perhaps justifying `Microsoft.Quantum.Canon.ApplyJointRotation`? | ||
|
|
||
| > Please add a bullet point including your alias, your review result (*approve*, *reject*, *comment*), and a comment (optional when result is *approve*). Alternatively, add a line to the PR discussion incl. a reference to this issue. | ||
|
|
||
| **Consensus**: Approved with suggestions | ||
|
|
||
| --- | ||
|
|
||
| ### Add MeasureEachZ operation to runtime implementation | ||
|
|
||
| **Proposal**: https://github.com/microsoft/qsharp-runtime/issues/1005 | ||
|
|
||
| **Reviews**: | ||
| - **Approved.** I think this is a pretty straightforward one, and works great for replacing the old `MultiM` with something more maintainable and easier to use. | ||
| > Please add a bullet point including your alias, your review result (*approve*, *reject*, *comment*), and a comment (optional when result is *approve*). Alternatively, add a line to the PR discussion incl. a reference to this issue. | ||
|
|
||
| **Consensus**: Approved | ||
|
|
||
| --- | ||
|
|
||
| ### Simple and consistent arithmetic API | ||
|
|
||
| **Proposal**: https://github.com/microsoft/QuantumLibraries/issues/423 | ||
|
|
||
| **Reviews**: | ||
|
|
||
| - **Comment.** | ||
| - I'm a bit concerned that names like `GreaterThan` read like functions (noun or adjective) rather than as operations. | ||
| - How does it look to chain carry-out qubits to the `carryIn` of subsequent calls? The asymmetry that carry inputs are separate arguments while carry outputs are the MSB of LE registers feels like it could make that difficult to chain such that an example may be helpful. | ||
| - I'd personally recommend staging this together with https://github.com/microsoft/QuantumLibraries/issues/337 to minimize the number of breaking changes and to make sure that both APIs are as consistent with each other as possible. #337 in particular feels like it could be helpful in making the input data types consistent not only across arithmetic APIs but other places where arithmetic data is used in general-purpose APIs. | ||
|
|
||
| > Please add a bullet point including your alias, your review result (*approve*, *reject*, *comment*), and a comment (optional when result is *approve*). Alternatively, add a line to the PR discussion incl. a reference to this issue. | ||
msoeken marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| **Consensus**: Re-schedule after addressing comments | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.