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

Add functionality to qml.transforms.insert #1980

Merged
merged 28 commits into from
Dec 17, 2021

Conversation

ankit27kh
Copy link
Contributor

@ankit27kh ankit27kh commented Dec 3, 2021

Context:
As discussed in #1876, some quality of life enhancements are made.
Description of the Change:
before argument can be used to specify whether to insert operation before or after
position can now take operations to insert only after specific gates
Benefits:
more functionality
Possible Drawbacks:
none
Related GitHub Issues:
Closes #1876

@codecov
Copy link

codecov bot commented Dec 3, 2021

Codecov Report

Merging #1980 (9d197e6) into master (d700acf) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1980      +/-   ##
==========================================
- Coverage   99.17%   99.16%   -0.01%     
==========================================
  Files         218      223       +5     
  Lines       16665    17090     +425     
==========================================
+ Hits        16527    16948     +421     
- Misses        138      142       +4     
Impacted Files Coverage Δ
pennylane/transforms/insert_ops.py 100.00% <100.00%> (ø)
pennylane/about.py 95.45% <0.00%> (-4.55%) ⬇️
qchem/pennylane_qchem/qchem/__init__.py 100.00% <0.00%> (ø)
qchem/pennylane_qchem/qchem/openfermion.py 100.00% <0.00%> (ø)
qchem/pennylane_qchem/_version.py 100.00% <0.00%> (ø)
qchem/pennylane_qchem/qchem/structure.py 97.65% <0.00%> (ø)
qchem/pennylane_qchem/qchem/obs.py 100.00% <0.00%> (ø)
pennylane/__init__.py 100.00% <0.00%> (+2.38%) ⬆️

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 d700acf...9d197e6. Read the comment docs.

@ankit27kh
Copy link
Contributor Author

Hi @trbromley, does this match what you expected? Can you review this?

@trbromley
Copy link
Contributor

Thanks @ankit27kh! We'll take a look through and give some feedback 👍

@@ -51,10 +51,11 @@ def test_multiwire_op(self):
with pytest.raises(ValueError, match="Only single-qubit operations can be inserted into"):
insert(qml.CNOT, [])(self.tape)

def test_invalid_position(self):
@pytest.mark.parametrize("pos", [1, ["all", qml.RY, int], "ABC", str])
def test_invalid_position(self, pos):
"""Test if a ValueError is raised when an invalid position is requested"""
with pytest.raises(ValueError, match="Position must be either 'start', 'end', or 'all'"):
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this line change to match the new error message raised by the ValueError (line 222 - 225)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

match is regex-based, and the beginning is the same for both old and new error messages. Thus, I left it as is.

@Jaybsoni
Copy link
Contributor

Hey @ankit27kh, looks good! Just a few small comments and this should be good.
Thanks for the contribution

ankit27kh and others added 3 commits December 14, 2021 02:10
Copy link
Contributor

@Jaybsoni Jaybsoni 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 to me!

@Jaybsoni
Copy link
Contributor

@antalszava,
It seems code factor is failing due to issues found in separate files, any suggestions on how to address this? Is there a PR open for cleaning those up?

@antalszava
Copy link
Contributor

It is being addressed in #2038.

@antalszava
Copy link
Contributor

antalszava commented Dec 17, 2021

Hi @ankit27kh it seems that the coverage report check is not passing. Could you confirm that it's a mistake by running make coverage locally and checking that the added lines are indeed covered?

@ankit27kh
Copy link
Contributor Author

Hey @antalszava, I ran it locally, and the relevant file is 100% covered.

Also, from here, it shows that the file losing coverage is about.py while insert_ops.py is fully covered.

pennylane/transforms/insert_ops.py 100.00% <100.00%> (ø)
pennylane/about.py 95.45% <0.00%> (-4.55%) arrow_down

@antalszava antalszava merged commit 995f04f into PennyLaneAI:master Dec 17, 2021
@ankit27kh ankit27kh deleted the patch-2 branch December 18, 2021 03:58
@josh146 josh146 mentioned this pull request Feb 4, 2022
11 tasks
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.

Insert only for certain gates when using qml.transforms.insert
4 participants