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

Adding tests for all PennyLane transforms #1215

Open
wants to merge 25 commits into
base: main
Choose a base branch
from
Open

Conversation

willjmax
Copy link
Contributor

This PR adds unit tests for all PennyLane transforms. This PR addresses [sc-72626].

Copy link

codecov bot commented Oct 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.95%. Comparing base (75dc517) to head (eb2182d).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1215      +/-   ##
==========================================
+ Coverage   96.72%   97.95%   +1.23%     
==========================================
  Files          56       77      +21     
  Lines        6800    11318    +4518     
  Branches      780      981     +201     
==========================================
+ Hits         6577    11087    +4510     
- Misses        173      181       +8     
  Partials       50       50              

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

Copy link
Contributor

Hello. You may have forgotten to update the changelog!
Please edit doc/releases/changelog-dev.md on your branch 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.

@paul0403
Copy link
Contributor

paul0403 commented Oct 18, 2024

Thanks for adding this, I will take a closer look in a bit, but first @mlxd , what's the guideline on large batches of xfailed python tests like this? Do we prefer skip over xfail, to save CI execution time?

For context, @willjmax is going over the core PL transforms and testing whether qjit works with them or not. If they don't work, a reason is noted in this testing file.

Edit: discussed this; if we know something fails, but it shouldn't, we should always aim to mark it xfail.
Skipping means we have no signal at all of whether it will pass or fail

Copy link
Contributor

@paul0403 paul0403 left a comment

Choose a reason for hiding this comment

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

Thanks for adding the tests 💯 Some questions from me

.dep-versions Outdated Show resolved Hide resolved
frontend/test/pytest/test_transform.py Outdated Show resolved Hide resolved
frontend/test/pytest/test_transform.py Outdated Show resolved Hide resolved
frontend/test/pytest/test_transform.py Outdated Show resolved Hide resolved
frontend/test/pytest/test_transform.py Outdated Show resolved Hide resolved
frontend/test/pytest/test_transform.py Outdated Show resolved Hide resolved
frontend/test/pytest/test_transform.py Outdated Show resolved Hide resolved
frontend/test/pytest/test_transform.py Outdated Show resolved Hide resolved
frontend/test/pytest/test_transform.py Outdated Show resolved Hide resolved
frontend/test/pytest/test_transform.py Outdated Show resolved Hide resolved
.dep-versions Outdated Show resolved Hide resolved
frontend/test/pytest/test_transform.py Outdated Show resolved Hide resolved
Copy link
Contributor

@erick-xanadu erick-xanadu left a comment

Choose a reason for hiding this comment

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

Thanks @willjmax ! Interesting results that we need to look at to make sure Catalyst and PL are more compatible with each other. Cheers!

Copy link

@soranjh soranjh left a comment

Choose a reason for hiding this comment

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

Thanks @willjmax, left some comments, will be good from my side after addressing them.


@pytest.mark.xfail(reason="Noise models not supported on Lightning.")
def test_add_noise(backend):
"""Test the add noise transform on a simple circuit"""
Copy link

Choose a reason for hiding this comment

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

Suggested change
"""Test the add noise transform on a simple circuit"""
"""Test the add_noise transform on a simple circuit"""

Comment on lines -124 to +168
def test_split_non_commuting(backend):
"""Test split non commuting"""
def test_batch_partial(backend):
"""Test batch partial"""
Copy link

Choose a reason for hiding this comment

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

Why this is changed?

Copy link
Contributor

Choose a reason for hiding this comment

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

@qml.transforms.split_non_commuting
@qml.qnode(qml.device(device_name, wires=6), interface="jax")
def qfunc():
"""Example taken from PL tests"""
Copy link

Choose a reason for hiding this comment

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

Fine to mention this but it is not the case for all other tests, why explicitly mentioned for this one?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this test was written by me. The reason why it is in green is because git sees it being deleted and rewritten instead of being move. We can also remove this comment if you think it is worthwhile.



@pytest.mark.xfail(reason="Noise models not supported on Lightning.")
def test_add_noise(backend):
Copy link

Choose a reason for hiding this comment

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

Where backend is defined?

Copy link
Contributor

Choose a reason for hiding this comment

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

The backend is a variable provided from the command line to pytest. It defaults to "lightning.qubit" but we also run it with "lightning.kokkos".

# pylint: disable=too-many-lines,line-too-long


@pytest.mark.xfail(reason="Noise models not supported on Lightning.")
Copy link

Choose a reason for hiding this comment

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

Will be better to move all xfail tagged tests to the end of file.

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.

6 participants