Break up qubit operations file #1467

albi3ro · 2021-07-21T16:40:17Z

Fixes #1454

This PR breaks ./pennylane/ops/qubit.py into 8 different files in a ./pennylane/ops/qubit/ folder.

The new categories are:

non parametric ops
parametric ops
qchem ops
state preparations
observables
arithmetic ops
arbitrary ops: things like Qubit Unitary
other: currently just QFT. couldn't figure out where to put it

github-actions · 2021-07-21T16:40:36Z

Hello. You may have forgotten to update the changelog!
Please edit .github/CHANGELOG.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.

…lane into break_up_qubit

codecov · 2021-07-21T19:05:46Z

Codecov Report

Merging #1467 (5826ed5) into master (69800c4) will decrease coverage by 0.00%.
The diff coverage is 98.65%.

@@            Coverage Diff             @@
##           master    #1467      +/-   ##
==========================================
- Coverage   98.31%   98.31%   -0.01%     
==========================================
  Files         172      179       +7     
  Lines       12599    12652      +53     
==========================================
+ Hits        12387    12439      +52     
- Misses        212      213       +1

Impacted Files	Coverage Δ
pennylane/ops/qubit/non_parametric_ops.py	`96.78% <96.78%> (ø)`
pennylane/ops/qubit/matrix_ops.py	`97.11% <97.11%> (ø)`
pennylane/ops/qubit/parametric_ops.py	`99.78% <99.78%> (ø)`
pennylane/ops/__init__.py	`100.00% <100.00%> (ø)`
pennylane/ops/qubit/__init__.py	`100.00% <100.00%> (ø)`
pennylane/ops/qubit/arithmetic_ops.py	`100.00% <100.00%> (ø)`
pennylane/ops/qubit/observables.py	`100.00% <100.00%> (ø)`
pennylane/ops/qubit/qchem_ops.py	`100.00% <100.00%> (ø)`
pennylane/ops/qubit/state_preparation.py	`100.00% <100.00%> (ø)`
... and 7 more

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 69800c4...5826ed5. Read the comment docs.

albi3ro · 2021-07-22T16:07:27Z

pennylane/ops/qubit/other.py

+from pennylane.operation import AnyWires, Operation
+
+
+class QFT(Operation):


Can anyone think of where this might fit better?

QFT actually feels more like a subroutine template rather than a regular qubit operation; maybe it could be moved there? (Otherwise, I think arithmetic is the closest match, but even that's not quite right)

Any reasoning as why it wasn't a subroutine template?

Good question - calling in @trbromley for this thoughts since he wrote the QFT 🙂

.github/CHANGELOG.md

antalszava · 2021-07-27T18:24:29Z

pennylane/ops/qubit/matrix_ops.py

+# please move it to ``non_parametric_ops.py``
+
+
+class MultiControlledX(ControlledQubitUnitary):


Hi @albi3ro, will be going for a full review. One thing before that is that it would be great to ensure that the reorganization is with respect to how the operations behave, not how they are coded. E.g., here MultiControlledX should probably live in non_parametric_ops rather than this file.

An external contributor is editing 'MultiControlledX' to no longer inherit from 'ControlledQubitUnitary', at which point it can be moved to non-parametric ops.

@albi3ro I would strongly recommend organizing the code around how the operation behaves/its category, rather than around how it is coded up 🤔

Even though MultiControlledX is a matrix op internally, I feel like this is an implementation detail? How an operation is implemented should ideally not require it to change files.

As another example, as a new dev I would expect to find CNOT in non_parametrized.py. However, if it was instead in matrix_ops (because it was implemented via the matrix directly), it would not be trivial for me to find it. Likewise, for a contributor making a change/improvement to an operation, requiring them to move its location sounds like an additional barrier, and would make the PR diff bigger!

thisac

Just a few minor comments and things that'd be good to address. Otherwise it looks great!

pennylane/ops/qubit/__init__.py

thisac · 2021-07-27T18:23:43Z

pennylane/ops/qubit/parametric_ops.py

+from pennylane.utils import expand, pauli_eigs
+from pennylane.wires import Wires
+
+from .non_parametric_ops import PauliX, PauliY, PauliZ, Hadamard, CNOT


This might be good to address still, just changing it to an absolute import. 🙂

thisac · 2021-07-27T18:26:32Z

pennylane/ops/qubit/state_preparation.py

+from pennylane.templates.state_preparations import BasisStatePreparation, MottonenStatePreparation
+
+
+class AdjointError(Exception):


Perhaps addressing this as well, moving the exception to the __init__.py file would make sense here, and simply import it from there.

pennylane/ops/qubit/non_parametric_ops.py

pennylane/ops/qubit/__init__.py

antalszava

Some comments left to be addressed, but looking on track overall to me!

Co-authored-by: antalszava <antalszava@gmail.com> Co-authored-by: Theodor <theodor@xanadu.ai>

albi3ro · 2021-07-28T16:49:26Z

I'll be making another PR in the near future where I focus on improving the operation tests. At that point I will bump up code code coverage, but for now we will be missing coverage on some lines.

pennylane/ops/__init__.py

antalszava

Looks good to me! 🎉 This will be so helpful in finding operations & observables! 😍 Thank you :)

pennylane/ops/qubit/__init__.py

.github/CHANGELOG.md

Co-authored-by: antalszava <antalszava@gmail.com>

thisac

This is great @albi3ro! Just added a few very minor (picky) suggestions to make it look a bit more like the rest of the files in the repo. 🙂

thisac · 2021-07-29T16:46:22Z

pennylane/ops/__init__.py

+class AdjointError(Exception):
+    """Exception for non-adjointable operations."""
+    pass


pennylane/ops/qubit/__init__.py

pennylane/ops/qubit/arithmetic_ops.py

pennylane/ops/qubit/matrix_ops.py

pennylane/ops/qubit/non_parametric_ops.py

pennylane/ops/qubit/parametric_ops.py

pennylane/ops/qubit/qchem_ops.py

Co-authored-by: Theodor <theodor@xanadu.ai>

albi3ro added 3 commits July 21, 2021 09:22

breaking up qubit.py

606cdb6

fixing imports

90db316

rename file to keep import paths

5a4f92d

albi3ro added code quality 🎓 Discussion and improvements to code quality WIP 🚧 Work-in-progress labels Jul 21, 2021

Merge branch 'master' into break_up_qubit

e2e12f3

albi3ro and others added 9 commits July 21, 2021 10:16

changelog, eliminate unused imports

2ac7203

fix merge

f7d0dfc

fix changelog, remove unused imports

41356a3

Merge branch 'master' into break_up_qubit

938997e

black

320b267

Merge branch 'break_up_qubit' of https://github.com/PennyLaneAI/penny…

ed87b98

…lane into break_up_qubit

really remove unused imports this time

bc522b4

remove old file, fix imports

4b5457a

fix tests and black

191b7c3

black

b958021

albi3ro removed the WIP 🚧 Work-in-progress label Jul 21, 2021

Merge branch 'master' into break_up_qubit

6c9aef8

albi3ro commented Jul 22, 2021

View reviewed changes

albi3ro added the review-ready 👌 PRs which are ready for review by someone from the core team. label Jul 22, 2021

albi3ro requested review from thisac and antalszava July 22, 2021 16:08

albi3ro mentioned this pull request Jul 26, 2021

Make QFT a template instead of an operation #1477

Closed

albi3ro added 5 commits July 26, 2021 10:07

Merge branch 'master' into break_up_qubit

025cf1b

fully merge in pr 1364

041cb12

fix imports

e30ddba

update toffoli controls

45ca3df

phase shift op

8b23f99

albi3ro added 3 commits July 27, 2021 09:48

oops

8bfdefc

fix imports

821de26

more fix imports

5d44582

albi3ro requested review from thisac and antalszava July 27, 2021 17:09

albi3ro mentioned this pull request Jul 27, 2021

Parameters in MultiControlledX #1452

Closed

docstring, arithmetic spelling

78172cb

albi3ro commented Jul 27, 2021

View reviewed changes

.github/CHANGELOG.md Outdated Show resolved Hide resolved

Update .github/CHANGELOG.md

7dce254

antalszava reviewed Jul 27, 2021

View reviewed changes

thisac reviewed Jul 27, 2021

View reviewed changes

antalszava reviewed Jul 28, 2021

View reviewed changes

pennylane/ops/qubit/__init__.py Outdated Show resolved Hide resolved

antalszava reviewed Jul 28, 2021

View reviewed changes

albi3ro and others added 5 commits July 28, 2021 07:59

Apply suggestions from code review

17aa692

Co-authored-by: antalszava <antalszava@gmail.com> Co-authored-by: Theodor <theodor@xanadu.ai>

review feedback

2579afc

black

2ef1ce8

fix adjoint error issues

d3f3091

minor wording

bbef72e

albi3ro requested review from thisac and antalszava July 28, 2021 16:33

antalszava reviewed Jul 29, 2021

View reviewed changes

pennylane/ops/__init__.py Show resolved Hide resolved

antalszava approved these changes Jul 29, 2021

View reviewed changes

pennylane/ops/qubit/__init__.py Outdated Show resolved Hide resolved

.github/CHANGELOG.md Outdated Show resolved Hide resolved

albi3ro and others added 3 commits July 29, 2021 07:59

Apply suggestions from code review

6df6a64

Co-authored-by: antalszava <antalszava@gmail.com>

Merge branch 'master' into break_up_qubit

c784a8b

black

b5c3312

thisac approved these changes Jul 29, 2021

View reviewed changes

Apply suggestions from code review

5826ed5

Co-authored-by: Theodor <theodor@xanadu.ai>

josh146 merged commit 5af5733 into master Jul 29, 2021

josh146 deleted the break_up_qubit branch July 29, 2021 17:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Break up qubit operations file #1467

Break up qubit operations file #1467

albi3ro commented Jul 21, 2021

github-actions bot commented Jul 21, 2021

codecov bot commented Jul 21, 2021 •

edited

Loading

albi3ro Jul 22, 2021

glassnotes Jul 25, 2021

albi3ro Jul 26, 2021

glassnotes Jul 26, 2021

antalszava Jul 27, 2021

albi3ro Jul 27, 2021

josh146 Jul 28, 2021 •

edited

Loading

thisac left a comment

thisac Jul 27, 2021

thisac Jul 27, 2021

antalszava left a comment

albi3ro commented Jul 28, 2021

antalszava left a comment

thisac left a comment

thisac Jul 29, 2021

		from pennylane.operation import AnyWires, Operation


		class QFT(Operation):

		# please move it to ``non_parametric_ops.py``


		class MultiControlledX(ControlledQubitUnitary):

		from pennylane.templates.state_preparations import BasisStatePreparation, MottonenStatePreparation


		class AdjointError(Exception):

Break up qubit operations file #1467

Break up qubit operations file #1467

Conversation

albi3ro commented Jul 21, 2021

github-actions bot commented Jul 21, 2021

codecov bot commented Jul 21, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josh146 Jul 28, 2021 • edited Loading

Choose a reason for hiding this comment

thisac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antalszava left a comment

Choose a reason for hiding this comment

albi3ro commented Jul 28, 2021

antalszava left a comment

Choose a reason for hiding this comment

thisac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 21, 2021 •

edited

Loading

josh146 Jul 28, 2021 •

edited

Loading