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

Cast gates to dense matrices in optools #414

Merged
merged 4 commits into from
Apr 4, 2024

Conversation

Timo1104
Copy link

@Timo1104 Timo1104 commented Mar 29, 2024

Description of changes

Minor change: in optools.entanglement_fidelity, accept both arrays and gates for the a and b arguments. The arguments are cast to (dense) arrays within the function scope.

Perform similar casting in other functions, that call a.shape.

Motivation

See also #406.

Since some update (cannot pinpoint which exactly), this casting was not done anymore, neither within optools.entanglement_fidelity, nor in any of the functions calling this one.

I've chosen to cast to dense array within optools.entanglement_fidelity, rather than before calling this function, for two reasons:

  • It keeps the number of changes as small as possible;
  • It provides a more consistent interface across optools. Other functions (such as average_gate_fidelity, which calls entanglement_fidelity) accept arrays or gates. Hence, I'm of the opinion that casting to array within entanglement_fidelity is the more consistent choice. Alternatively, casting can be done before calling it, but (IMO) it's more consistent to then also do that for the other metrics functions defined in optools. I'm okay with either way, but this seems the simpler and less backwards-breaking solution, but I'm happy to implemetn otherwise if required.
  • It resolves an error in for example average_gate_fidelity, which states in the docstring that it accepts arrays or gates for a and b, but directly passes a and b to entanglement_fidelity, leading to an error.
  • Prevents errors on calling a.shape on gates that do not have the shape attribute (such as ComposedOps)

@Timo1104 Timo1104 requested review from a team as code owners March 29, 2024 13:39
@Timo1104 Timo1104 changed the title Cast gates to dense matrices in entanglement_fidelity Cast gates to dense matrices in optools Mar 29, 2024
Copy link
Contributor

@sserita sserita left a comment

Choose a reason for hiding this comment

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

Will approve once the contextlib calls are swapped out for LinearOperator checks.

Thanks for your PR!

@@ -430,6 +433,13 @@ def entanglement_fidelity(a, b, mx_basis='pp', is_tp=None, is_unitary=None):
-------
float
"""
# Attempt to cast to dense array. If this is already an array, the AttributeError
# will be suppressed.
with _contextlib.suppress(AttributeError):
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we change the contextlib.suppress calls to an isinstance(_, LinearOperator) instead?

It's more pyGSTi style to check for the base class that provides the used API than a blanket error suppression.

Copy link
Author

Choose a reason for hiding this comment

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

Of course! I'm more used to EAFP-style programming, but happy to accomodate LBYL, too :) Just pushed the changes!

@Timo1104 Timo1104 requested a review from sserita April 4, 2024 06:10
Copy link
Contributor

@sserita sserita left a comment

Choose a reason for hiding this comment

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

Thanks Timo. There's definitely arguments to be made that EAFP is a nicer way to go, but it's appreciated that you're willing to conform to current style :)

Looks good, thanks for the contribution!

@sserita sserita merged commit 3c2ae0b into sandialabs:develop Apr 4, 2024
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.

2 participants