Minor doc fixes #190

numpde · 2019-12-16T16:47:23Z

No description provided.

msftclas · 2019-12-16T16:47:36Z

All CLA requirements met.

cgranade

Looks good, thanks! Just had some minor comments here and there that would be good to address before bringing this in. Thank you!

Standard/src/Arithmetic/Modular.qs

cgranade · 2019-12-16T17:57:47Z

Standard/src/Arithmetic/Modular.qs

    ///
    /// # See Also
-    /// - Microsoft.Quantum.Canon.ModularIncrementLE
+    /// - Microsoft.Quantum.Canon.ModularIncrementLE (deprecated)


The # See Also block doesn't support additional information such as deprecation notices, unfortunately. In this case, perhaps it would be better to change the See Also link instead?

Suggested change

/// - Microsoft.Quantum.Canon.ModularIncrementLE (deprecated)

/// - Microsoft.Quantum.Arithmetic.ModularIncrementByInteger

Maybe it is still worthwhile to reference the deprecated implementation in the # Remarks block.

Then, # See Also of IncrementPhaseByModularInteger should reference IncrementByModularInteger, and vice versa.

(Being new to this, I don't know how to take it from here; do I mark this as "resolved" and initiate a new PR?)

With the new @Deprecated() attribute, we've definitely improved on our ability to track deprecation and offer better warnings to users. Given that, I would suggest that /// # See Also blocks only refer to non-deprecated operations and functions, so as to present a consistent view of how the libraries evolve going forward.

With respect to moving forward with your contribution, there's no need to make a new PR, as this PR will update as you make changes to your numpde:patch-2 branch. For instance, if you use the "Commit suggestion" button above, that will create a new commit on your branch co-authored by the two of us, updating your PR accordingly. We have a few more details up in the contribution guide, but I'm also happy to answer anything that I can.

Is there a preferred way, <xref:uid_of_the_topic> vs @"uid_of_the_topic"?

I've gone over it again.

cgranade · 2019-12-16T17:58:17Z

Standard/src/Arithmetic/Modular.qs

    ///
    /// # See Also
-    /// - Microsoft.Quantum.Canon.ModularAddProductLE
+    /// - Microsoft.Quantum.Canon.ModularAddProductLE (deprecated)


Suggested change

/// - Microsoft.Quantum.Canon.ModularAddProductLE (deprecated)

/// - Microsoft.Quantum.Arithmetic.MultiplyAndAddByModularInteger

cgranade · 2019-12-16T17:59:01Z

Standard/src/Arithmetic/Modular.qs


    /// # Summary
-    /// The same as ModularAddProductLE, but assumes that summand encodes
+    /// The same as ModularAddProductLE (deprecated), but assumes that summand encodes


Similarly here, it might help to change the reference to point to the new name of the ModularAddProductLE operation:

Suggested change

/// The same as ModularAddProductLE (deprecated), but assumes that summand encodes

/// The same as MultiplyAndAddByModularInteger, but assumes that summand encodes

There are probably still some unstated assumptions on the algorithms.

cgranade

Apologies for my delay over the holidays, but I think this is getting a lot closer. Thanks again for your contribution, and for your patience with our documentation pipeline. We really appreciate it!

cgranade · 2019-12-18T17:47:36Z

Standard/src/Arithmetic/Modular.qs

    /// # Summary
-    /// The same as ModularAddProductLE (deprecated), but assumes that summand encodes
-    /// integers in QFT basis
+    /// The same as @"MultiplyAndAddByModularInteger", but assumes that the summand encodes


Unfortunately, the @"" notation refers to unique documentation identifiers (UIDs) rather than fully qualified names. At the moment, the documentation generation tool assigns UIDs as the lowercasing of fully qualified names, such that this would be @"microsoft.quantum.arithmetic.multiplyandaddbymodularinteger". That said, we generally discourage links from appearing in summaries at all (rather, preferencing the /// # Description section) since summary information gets pulled into hover information, and since not all clients support links in hovers.

Standard/src/Arithmetic/Modular.qs

...because there is inline math. Co-Authored-By: Chris Granade <cgranade@gmail.com>

Co-Authored-By: Chris Granade <cgranade@gmail.com>

numpde · 2020-01-04T12:06:31Z

Thanks for your comments; another iteration is in.

cgranade

Thanks for doing this, I really appreciate it! 💕

* Clarify the restriction on the number of bits for IntAsBoolArray (#171) * Clarify the restriction on the number of bits for IntAsBoolArray This should fix #166 by providing a more specific error message. * Update Standard/src/Convert/Convert.qs Co-Authored-By: Chris Granade <chgranad@microsoft.com> * Allow to have bits = 0 Looks like our tests assume that number = 0 with bits = 0 is a valid scenario; updating the change to account for that * Package updates (#188) * Quantum AND gates (a.k.a. CCNOT with constant target) (#186) * Two AND gate implementations. * Added test case. * Formatting. * Code formatting. * Update Standard/src/Canon/And.qs Co-Authored-By: Chris Granade <chgranad@microsoft.com> * Assertion for 0-target. * Added DOI to references. * Named application for CCNOTop. * Rename operations. * Add Test attribute. * Add links to arXiv. * Rename operations. * Better assertion for 0-target. * Fix bug in LowDepthAnd. * Docs. * Doc string convention. * Controlled variant for `ApplyAnd`. * Controlled AndLowDepth. * Adjoint Controlled LowDepthAnd. * References. * Simplify code. * Apply suggestions from code review Co-Authored-By: Chris Granade <chgranad@microsoft.com> * Integrate comment. * Removed comment ref to IncrementByIntegerPhaseLE (#189) There appears to be no function IncrementByIntegerPhaseLE, and I guess it is covered by ApplyLEOperationOnPhaseLE. Co-authored-by: Chris Granade <cgranade@gmail.com> * New Hadamard and SWAP test operations. (#196) * First work on Hadamard and SWAP test operations. * (c) header and typo fix. * Fixed typo with placement of phase shift. * Put public operations above private. * Added tests for new operations. * Added API documentation comments. * Newline at end of file. * Refactor AA namespace to use Q# style guide (#197) * Began simplifying AA interface. * Expose traditional AA as new public operation. * Removed rest of "AmpAmp" prefix. * Resolve deprecation warning. * Switching to using the new Sdk (#194) * Minor doc fixes (#190) * Minor doc fixes * Minor doc cleanup There are probably still some unstated assumptions on the algorithms. * Add "# Description" for MultiplyByModularInteger ...because there is inline math. Co-Authored-By: Chris Granade <cgranade@gmail.com> * "unitary operation" instead of "unitary operator" Co-Authored-By: Chris Granade <cgranade@gmail.com> * Add "# Description", remove refs in "# Summary" Co-authored-by: Chris Granade <cgranade@gmail.com> * Fix build by updating QML projects to use SDK. Co-authored-by: Mariia Mykhailova <michaylova@gmail.com> Co-authored-by: bettinaheim <34236215+bettinaheim@users.noreply.github.com> Co-authored-by: Mathias Soeken <mathias.soeken@gmail.com> Co-authored-by: numpde <21158052+numpde@users.noreply.github.com>

This reverts commit f2bb611.

Minor doc fixes

ad2e405

cgranade mentioned this pull request Dec 16, 2019

Documentation generation tool not resolving namespaces correctly in # See Also blocks. microsoft/qsharp-compiler#267

Open

cgranade suggested changes Dec 16, 2019

View reviewed changes

Minor doc cleanup

37266fe

There are probably still some unstated assumptions on the algorithms.

cgranade suggested changes Dec 27, 2019

View reviewed changes

Chris Granade and others added 4 commits December 30, 2019 11:16

Merge branch 'master' into patch-2

fd095bc

Add "# Description" for MultiplyByModularInteger

760fb12

...because there is inline math. Co-Authored-By: Chris Granade <cgranade@gmail.com>

"unitary operation" instead of "unitary operator"

22bdd26

Co-Authored-By: Chris Granade <cgranade@gmail.com>

Add "# Description", remove refs in "# Summary"

bf85a85

Merge branch 'master' into patch-2

ee7292f

cgranade approved these changes Jan 6, 2020

View reviewed changes

cgranade merged commit f2bb611 into microsoft:master Jan 7, 2020

bettinaheim pushed a commit that referenced this pull request Jan 10, 2020

Revert "Minor doc fixes (#190)"

71070c8

This reverts commit f2bb611.

	/// - Microsoft.Quantum.Canon.ModularIncrementLE (deprecated)
	/// - Microsoft.Quantum.Arithmetic.ModularIncrementByInteger

	/// - Microsoft.Quantum.Canon.ModularAddProductLE (deprecated)
	/// - Microsoft.Quantum.Arithmetic.MultiplyAndAddByModularInteger

	/// The same as ModularAddProductLE (deprecated), but assumes that summand encodes
	/// The same as MultiplyAndAddByModularInteger, but assumes that summand encodes

Minor doc fixes #190

Minor doc fixes #190

Uh oh!

Conversation

numpde commented Dec 16, 2019

Uh oh!

msftclas commented Dec 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cgranade left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cgranade left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

numpde commented Jan 4, 2020

Uh oh!

cgranade left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

msftclas commented Dec 16, 2019 •

edited

Loading