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

[unitaryhack] Remove qml.utils.expand and replace with qml.operation.expand_matrix. #2654

Merged
merged 8 commits into from
Jun 10, 2022

Conversation

maezyn
Copy link
Contributor

@maezyn maezyn commented Jun 3, 2022

Context:
Removes the utility function qml.utils.expand and replaces all references to it with qml.operation.expand_matrix.
Description of the Change:

  • Removes qml.utils.expand and its corresponding tests.
  • Replaces all references of qml.utils.expand with qml.operation.expand_matrix.
  • Renames TestExpand to TestExpandVector in test_utils.py to better reflect the remaining test cases.

Benefits:
Removes the utility function qml.utils.expand which is no longer used anywhere as of #2609.
Possible Drawbacks:
Removes qml.utils.expand tests from test_utils.py.
Related GitHub Issues:
Closes #2637

@maezyn maezyn marked this pull request as ready for review June 3, 2022 02:40
@codecov
Copy link

codecov bot commented Jun 3, 2022

Codecov Report

Merging #2654 (97a08af) into master (5d83fdf) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2654      +/-   ##
==========================================
- Coverage   99.61%   99.61%   -0.01%     
==========================================
  Files         251      251              
  Lines       20682    20662      -20     
==========================================
- Hits        20602    20582      -20     
  Misses         80       80              
Impacted Files Coverage Δ
pennylane/operation.py 96.59% <ø> (ø)
pennylane/ops/qubit/parametric_ops.py 100.00% <100.00%> (ø)
pennylane/utils.py 100.00% <100.00%> (ø)

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 5d83fdf...97a08af. Read the comment docs.

@josh146
Copy link
Member

josh146 commented Jun 6, 2022

thanks @mmore21 for this contribution! Is it ready for review?

@antalszava antalszava self-assigned this Jun 7, 2022
@maezyn
Copy link
Contributor Author

maezyn commented Jun 8, 2022

thanks @mmore21 for this contribution! Is it ready for review?

Hi @josh146 and @antalszava, should be ready for review. Thanks!

Copy link
Contributor

@antalszava antalszava left a comment

Choose a reason for hiding this comment

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

Hi @mmore21, this is looking great, thank you for the contribution! 🙂

One thing before approval would be to extend the functionality such that qml.utils.expand is still valid, though it resolves to qml.operation.expand_matrix. Although this PR addresses all usages of qml.utils.expand, it may be that users would have their codes breaking if we made the removal in one step.

Adding the following code to the beginning of the code section of pennylane/utils.py should help:

import warnings

def __getattr__(name):
    # for more information on overwriting `__getattr__`, see https://peps.python.org/pep-0562/
    if name == "expand":
        warning_string = f"qml.utils.expand is deprecated; using qml.operation.expand_matrix instead."
        warnings.warn(warning_string, UserWarning)
        return qml.operation.expand_matrix
    try:
        return globals()[name]
    except KeyError as e:
        raise AttributeError from e

It would also be good to include a test case for this, just to make sure that a warning is raised.

Apart from this, the rest of the changes are looking great. 👍

doc/releases/changelog-dev.md Outdated Show resolved Hide resolved
Copy link
Contributor

@antalszava antalszava 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 @mmore21! 🎉 Thank you so much for the contribution! 😊 Very neat 👏

@maezyn
Copy link
Contributor Author

maezyn commented Jun 9, 2022

Thanks @antalszava, happy to make my first contribution!

@antalszava
Copy link
Contributor

Hi @mmore21 great to have you contribute 🙂

As you've made your contribution during Unitary HACK you are eligible for swag. 🙂 To claim it you should be signed up for UnitaryHACK here and add [unitaryhack] in the title of the PR.

Let me know if you have further questions (cc @CatalinaAlbornoz).

@maezyn maezyn changed the title Remove qml.utils.expand and replace with qml.operation.expand_matrix. Remove qml.utils.expand and replace with qml.operation.expand_matrix. [unitaryhack] Jun 9, 2022
@maezyn
Copy link
Contributor Author

maezyn commented Jun 9, 2022

Awesome, just registered and updated the title. Thanks for letting me know @antalszava!

@antalszava
Copy link
Contributor

Overriding the checks: coverage decrease is due to deleted lines, docs build is lagging due to concurrency issues.

@antalszava antalszava merged commit fa9f858 into PennyLaneAI:master Jun 10, 2022
@antalszava antalszava changed the title Remove qml.utils.expand and replace with qml.operation.expand_matrix. [unitaryhack] [unitaryhack] Remove qml.utils.expand and replace with qml.operation.expand_matrix. Jun 22, 2022
@antalszava antalszava added the unitaryhack-accepted This contribution has been accepted for a UnitaryHack issue label Jun 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
unitaryhack-accepted This contribution has been accepted for a UnitaryHack issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove qml.utils.expand
3 participants