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

move draw and draw_mpl to drawer module #2396

Merged
merged 9 commits into from
Mar 31, 2022
Merged

move draw and draw_mpl to drawer module #2396

merged 9 commits into from
Mar 31, 2022

Conversation

albi3ro
Copy link
Contributor

@albi3ro albi3ro commented Mar 31, 2022

Currently, qml.draw and qml.draw_mpl are located within the transforms module, as they do indeed take a qnode and turn it into a function that returns something else.

BUT, the two functions only depend on code in the drawer module and nothing in the transforms module. Nothing else in the transforms module depends on these methods.

Yes, structurally they look like other things in the transforms module, but there is an entire module dedicated to the concept of drawing. I argue that the concept of drawing is more important than a common structural pattern.

This is an internal organization change only and has no user-facing implications.

@@ -200,6 +200,10 @@
* Circuit cutting now performs expansion to search for wire cuts in contained operations or tapes.
[(#2340)](https://github.com/PennyLaneAI/pennylane/pull/2340)

* The `qml.draw` and `qml.draw_mpl` transforms are now located in the `drawer` module. They can still be
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should this be considered a breaking change? It shouldn't have any user facing ramifications.

Copy link
Contributor

Choose a reason for hiding this comment

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

What comes to mind is that it could be breaking if there were occurrences of qml.transforms.draw anywhere in our codebase or in user code.

Comment on lines -26 to -27
from .drawable_layers import drawable_layers
from .utils import convert_wire_order
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These two methods are only accessed internally by tape_text and tape_mpl. I don't see a reason we need to make them accessible from here.

@codecov
Copy link

codecov bot commented Mar 31, 2022

Codecov Report

Merging #2396 (6fc9402) into master (9fe4638) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #2396   +/-   ##
=======================================
  Coverage   99.42%   99.42%           
=======================================
  Files         243      243           
  Lines       18864    18864           
=======================================
  Hits        18756    18756           
  Misses        108      108           
Impacted Files Coverage Δ
pennylane/transforms/__init__.py 100.00% <ø> (ø)
pennylane/__init__.py 100.00% <100.00%> (ø)
pennylane/drawer/__init__.py 100.00% <100.00%> (ø)
pennylane/drawer/draw.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 9fe4638...6fc9402. Read the comment docs.

Comment on lines -55 to -58
Developer Methods
-----------------

.. automodapi:: pennylane.drawer
Copy link
Contributor

Choose a reason for hiding this comment

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

How come this was changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

draw and draw_mpl are not developer methods. In fact, they're the most important methods of the module, so I thought it would be important to move them to the top.

I think tape_text and tape_mpl are on the boundary between user-facing and developer-only. They have quite a bit of use for anyone working primarily with tapes instead of just qnodes.

@@ -14,15 +14,14 @@
"""
This module provides the circuit drawing functionality used to display circuits visually.

.. currentmodule:: pennylane.circuit_drawer
.. currentmodule:: pennylane.drawer
Copy link
Contributor

Choose a reason for hiding this comment

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

How come this name changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This name changed happened a few months ago. Seems like this line just slipped through that PR.

Copy link
Contributor

@antalszava antalszava left a comment

Choose a reason for hiding this comment

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

I argue that the concept of drawing is more important than a common structural pattern.

No strong opinions about this, unless any objections arise, happy to approve once the small questions are answered.

Copy link
Contributor

@antalszava antalszava left a comment

Choose a reason for hiding this comment

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

Looks good 👍

@albi3ro albi3ro merged commit e2a82c5 into master Mar 31, 2022
@albi3ro albi3ro deleted the move-draw-trasnform branch March 31, 2022 21:53
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.

None yet

2 participants