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

Incorporate level keyword in draw and draw_mpl #5855

Merged
merged 29 commits into from
Jun 19, 2024
Merged

Conversation

Shiro-Raven
Copy link
Collaborator

@Shiro-Raven Shiro-Raven commented Jun 13, 2024

Context:
Currently, draw() and draw_mpl() can only be requested after applying the full transform program, with the exception of the stages provided through expansion_strategy.

Description of the Change:
Using the new level argument from construct_batch, the functions are adapted to make use of the argument as well, which allows for more flexible requests and ability to pinpoint where exactly in the transform program to draw a circuit. As done before, the new functionality works with transforms that split the tape (only in the case for draw. For draw_mpl, a warning is raised and only the first tape is plotted).

Benefits:
Better plotting UX.

Note for Reviewers:
Minor bugs in construct_batch have been discovered during work on this PR, and so expect minor fixes to tests relating to specs.

[sc-53735] Supersedes #5139

Copy link
Contributor

Hello. You may have forgotten to update the changelog!
Please edit doc/releases/changelog-dev.md with:

  • A one-to-two sentence description of the change. You may include a small working example for new features.
  • A link back to this PR.
  • Your name (or GitHub username) in the contributors section.

Copy link
Contributor

@mudit2812 mudit2812 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 @Shiro-Raven 🎉 ! I don't consider the comments in this review to be blocking. I'll approve once tests are passing.

pennylane/drawer/draw.py Outdated Show resolved Hide resolved
@Shiro-Raven Shiro-Raven added this to the v0.37 milestone Jun 18, 2024
pennylane/drawer/draw.py Outdated Show resolved Hide resolved
pennylane/drawer/draw.py Outdated Show resolved Hide resolved
pennylane/drawer/draw.py Show resolved Hide resolved
Shiro-Raven and others added 4 commits June 18, 2024 16:14
…nd PT

* Fix `specs` tests relating to `trainable_params` fix
* Fix bug in PyTorch's `constuct`
Co-authored-by: Thomas R. Bromley <49409390+trbromley@users.noreply.github.com>
Co-authored-by: Thomas R. Bromley <49409390+trbromley@users.noreply.github.com>
Copy link

codecov bot commented Jun 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.66%. Comparing base (b38fe79) to head (ba3f99d).
Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5855      +/-   ##
==========================================
+ Coverage   99.43%   99.66%   +0.23%     
==========================================
  Files         421      421              
  Lines       41191    40309     -882     
==========================================
- Hits        40958    40175     -783     
+ Misses        233      134      -99     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pennylane/drawer/draw.py Outdated Show resolved Hide resolved
Copy link
Contributor

@mudit2812 mudit2812 left a comment

Choose a reason for hiding this comment

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

Thanks @Shiro-Raven !

@albi3ro
Copy link
Contributor

albi3ro commented Jun 19, 2024

Looks like there's some code cov failures. Seems like some of the code in the qnn module was only tested via drawing, and is no longer hit. We could either delete those lines or write a unit test for calling layer.construct.

Copy link
Contributor

@albi3ro albi3ro left a comment

Choose a reason for hiding this comment

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

🎉 I'll be happy to approve once we resolve the codecov issues. I think we should just be able to add tests for each of the two layers where we call .construct(args, kwargs) and then double check that a tape was constructed.

Copy link
Contributor

@albi3ro albi3ro left a comment

Choose a reason for hiding this comment

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

🎉

@Shiro-Raven Shiro-Raven enabled auto-merge (squash) June 19, 2024 20:02
@Shiro-Raven Shiro-Raven merged commit a0871e5 into master Jun 19, 2024
40 checks passed
@Shiro-Raven Shiro-Raven deleted the ad/level-in-draw branch June 19, 2024 20:44
mudit2812 added a commit that referenced this pull request Jul 2, 2024
**Context:**
Currently, `draw()` and `draw_mpl()` can only be requested after
applying the full transform program, with the exception of the stages
provided through expansion_strategy.

**Description of the Change:**
Using the new `level` argument from construct_batch, the functions are
adapted to make use of the argument as well, which allows for more
flexible requests and ability to pinpoint where exactly in the transform
program to draw a circuit. As done before, the new functionality works
with transforms that split the tape (only in the case for `draw`. For
`draw_mpl`, a warning is raised and only the first tape is plotted).

**Benefits:**
Better plotting UX.

**Note for Reviewers:**
Minor bugs in `construct_batch` have been discovered during work on this
PR, and so expect minor fixes to tests relating to `specs`.

[[sc-53735](https://app.shortcut.com/xanaduai/story/53735)] Supersedes
#5139

---------

Co-authored-by: Mudit Pandey <mudit.pandey@xanadu.ai>
Co-authored-by: Thomas R. Bromley <49409390+trbromley@users.noreply.github.com>
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

4 participants