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

Correct batch transform signatures in documentation #1733

Merged
merged 5 commits into from
Oct 10, 2021

Conversation

josh146
Copy link
Member

@josh146 josh146 commented Oct 9, 2021

Context: Currently, batch transformations created with the @qml.batch_transform decorator do not show the correct signature in the docs:

image

This is due to a drawback of functools.wraps --- it only wraps the docstring, not the signature.

Description of the Change: The @qml.batch_decorator is modified to simply return the input function, with no decoration, if it detects a Sphinx build (SPHINX_BUILD = "1").

Benefits:

The correct signature is now shown in the docs:

image

Possible Drawbacks:

If a user for some reason has SPHINX_BUILD = "1" in their environment, batch transforms will fail to work on QNodes. To help avoid this, a warning is raised in this situation.

Related GitHub Issues:

@josh146 josh146 added the documentation 📘 Documentation changes and updates label Oct 9, 2021
@codecov
Copy link

codecov bot commented Oct 9, 2021

Codecov Report

Merging #1733 (881b453) into master (5be2a8d) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1733   +/-   ##
=======================================
  Coverage   99.21%   99.21%           
=======================================
  Files         204      204           
  Lines       15425    15433    +8     
=======================================
+ Hits        15304    15312    +8     
  Misses        121      121           
Impacted Files Coverage Δ
pennylane/transforms/batch_transform.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5be2a8d...881b453. Read the comment docs.

Copy link
Contributor

@dwierichs dwierichs left a comment

Choose a reason for hiding this comment

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

No major comments from my side, good catch!

Both, the changelog and the __new__ method could have a warning regarding the unlikely case of a user having SPHINX_BUILD="1" in their environment.

Comment on lines +325 to +327
* The `qml.batch_transform` decorator is now ignored during Sphinx builds, allowing
the correct signature to display in the built documentation.
[(#1733)](https://github.com/PennyLaneAI/pennylane/pull/1733)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could add a word of warning how the detection of a sphinx build is done technically, so that users can quickly identify in case they run into the (unlikely) case of SPHINX_BUILD="1" in their environment?

Copy link
Member Author

@josh146 josh146 Oct 9, 2021

Choose a reason for hiding this comment

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

@dwierichs good idea! I added a warning here: 881b453

tape2 = tape.copy()
return [tape1, tape2], None

assert isinstance(my_transform, qml.batch_transform)
Copy link
Contributor

Choose a reason for hiding this comment

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

Very nice to test both cases!

@josh146 josh146 merged commit 557e170 into master Oct 10, 2021
@josh146 josh146 deleted the batch-transform-docs branch October 10, 2021 06:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation 📘 Documentation changes and updates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants