pennylane icon indicating copy to clipboard operation
pennylane copied to clipboard

Measured wires on qscript

Open comp-phys-marc opened this issue 2 years ago • 7 comments

Before submitting

Please complete the following checklist when submitting a PR:

  • [✓] All new features must include a unit test. If you've fixed a bug or added code that should be tested, add a test to the test directory!

  • [✓] All new functions and code must be clearly commented and documented. If you do make documentation changes, make sure that the docs build and render correctly by running make docs.

  • [✓] Ensure that the test suite passes, by running make test.

  • [✓] Add a new entry to the doc/releases/changelog-dev.md file, summarizing the change, and including a link back to the PR.

  • [✓] The PennyLane source code conforms to PEP8 standards. We check all of our code against Pylint. To lint modified files, simply pip install pylint, and then run pylint pennylane/path/to/file.py.

When all the above are checked, delete everything above the dashed line and fill in the pull request template.


Context: A nice-to-have feature of the QuantumScript requested in issue #3203.

Description of the Change: Adds a cached property to QuantumScript for the measured wires.

Benefits: The wires can be referenced instead of re-calculated, for example in _update_observables and QuantumScript().measured_wires.

Possible Drawbacks: The data footprint of a QuantumScript object is now larger.

Related GitHub Issues: https://github.com/PennyLaneAI/pennylane/issues/3203

comp-phys-marc avatar Jan 20 '23 06:01 comp-phys-marc

Thanks @comp-phys-marc tada

Make sure you also add your name to the contributors section at the bottom of the changelog and add a link to the entry. This kind of change should also fall under "Improvements" rather than "New features". New Features tend to be larger, user-focused things.

While this is how we currently tend to treat a lot of the properties in QuantumScript, this may be a good place where we can start using a more "lazy" approach to the properties.

We currently create a lot of "intermediate" quantum scripts, that are then transformed into something else. So for the vast majority of quantum scripts created, we won't be ever using measured_wires, so computing it is wasted time.

So I'd like to consider only computing measured_wires when we actually are going to use it. It's fairly lightweight, so this probably isn't that big of a deal, but might as well anyway.

Some options for being more lazy:

1. Recalculate it every time we need it: See [`samples_computational_basis`](https://github.com/PennyLaneAI/pennylane/blob/597beac35f49874a042f6e0dc61bdff69cce19a3/pennylane/tape/qscript.py#L313)

2. Leave `_measured_wires` as `None` till we calculate it for the first time: See [`specs`](https://github.com/PennyLaneAI/pennylane/blob/597beac35f49874a042f6e0dc61bdff69cce19a3/pennylane/tape/qscript.py#L1131) or `graph`.

3. Use `functools.cached_property`.  I haven't tried this out yet myself. But its an option.

I'm leaning toward option 1 till we start actually using the measured_wires property more often.

Thanks so much for your feedback! I've made the property "lazy-loaded". I've also moved the new line in the Changelog to the Improvements section, added my name and addressed the rest of your notes. :)

comp-phys-marc avatar Jan 20 '23 20:01 comp-phys-marc

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 99.66%. Comparing base (248a808) to head (d0b5ed4).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3655      +/-   ##
==========================================
- Coverage   99.67%   99.66%   -0.01%     
==========================================
  Files         422      422              
  Lines       40655    40369     -286     
==========================================
- Hits        40522    40235     -287     
- Misses        133      134       +1     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Jan 20 '23 22:01 codecov[bot]

Just looking for a final review at this stage.

comp-phys-marc avatar Apr 07 '23 19:04 comp-phys-marc

Thanks @comp-phys-marc! I've let the team know that this PR is review ready.

josh146 avatar Apr 09 '23 18:04 josh146

Hello, I have updated and merged in main. I am wondering if there is anything else that needs be done for this PR to be reviewed? All checks are passing @timmysilv @albi3ro. Thank you. :)

comp-phys-marc avatar Jun 19 '24 23:06 comp-phys-marc

Hi @comp-phys-marc! Thanks a lot for opening this PR and returning back to it after a few months.

We've had an internal discussion about the utility of having a measured wires attribute, as requested in the original #3203 issue. Since that issue was opened, things have evolved a bit. In fact, we're moving towards a new program representation - you can check it out here. While QuantumScript will still exist for the foreseeable future, we're cautious about changing it up during this transition period because we'd like to avoid the entropy and additional maintenance burden.

Because of this, we think that it would be best to close #3203 and this PR. Sorry for this, we know that it might be a bit frustrating given the original request in #3203. However, we still love to work with contributors like yourself and could suggest a fresher issue to work on if you would like.

trbromley avatar Jun 24 '24 14:06 trbromley

Hello, thanks so much for getting back to me on this! It's no problem, and it's exciting that you are moving to a new program representation. Please do feel free to suggest a fresher issue for me to work on. :)

comp-phys-marc avatar Jun 24 '24 16:06 comp-phys-marc