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

Split AnnotatedQueue from QueuingContext #2794

Merged
merged 15 commits into from
Sep 15, 2022
Merged

Conversation

albi3ro
Copy link
Contributor

@albi3ro albi3ro commented Jul 5, 2022

Currently, QuantumTape inherits from AnnotatedQueue which inherits from QueuingContext. This leads to a great deal of confusion about where functionality is defined and what methods are supposed to be doing.

This PR does not change any PennyLane behaviour, but just improves internal structure and clarity.

In this PR,

  • QueuingContext is no longer an abstract base class
  • AnnotatedQueue no longer inherits from QueuingContext
  • QueuingContext purely consists of class methods and a private class property.
  • QueuingContext is no longer a context. It only manages recording contexts. We should later rename it to QueueManager
  • QueuingContext._active_contexts is now a list instead of a deque. Deque was confusing overkill when a list gets the job done perfectly well
  • The private methods _append, _remove, _get_info, _update_info, and _safe_update_info are removed. They just caused confusing indirection.
  • AnnotatedQueue and QuantumTape will no longer provide global information about actively recording queues. This information was unused via those classes and just caused unnecessary duplication.
  • The tests for OperationRecorder are now in tests/tape/test_operation_recorder.py, mirroring the location of the class in the code base.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@albi3ro albi3ro requested a review from mlxd July 5, 2022 17:42
@codecov
Copy link

codecov bot commented Jul 5, 2022

Codecov Report

Merging #2794 (9b84ae3) into master (ae8ed09) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2794      +/-   ##
==========================================
- Coverage   99.67%   99.67%   -0.01%     
==========================================
  Files         272      272              
  Lines       23133    23122      -11     
==========================================
- Hits        23057    23046      -11     
  Misses         76       76              
Impacted Files Coverage Δ
pennylane/queuing.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@albi3ro albi3ro added the review-ready 👌 PRs which are ready for review by someone from the core team. label Jul 5, 2022
Copy link
Member

@mlxd mlxd left a comment

Choose a reason for hiding this comment

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

Thanks @albi3ro
Just a quick suggestion, but feel free to ignore if it isn't worth while.

Copy link
Contributor

@AlbertMitjans AlbertMitjans left a comment

Choose a reason for hiding this comment

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

The more I see it the more I like it! These changes really needed to be done!

Amazing job! 💯 🚀

@josh146 josh146 merged commit dbebb31 into master Sep 15, 2022
@josh146 josh146 deleted the queuing-inheritance-split branch September 15, 2022 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review-ready 👌 PRs which are ready for review by someone from the core team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants