sandialabs / pyGSTi

A python implementation of Gate Set Tomography
http://www.pygsti.info
Apache License 2.0
134 stars 56 forks source link

TypeError when performing gauge optimization with respect to fidelity or trace distance #406

Closed rileyjmurray closed 6 months ago

rileyjmurray commented 7 months ago

Running gauge optimization with respect to fidelity hits the following codeblock: https://github.com/sandialabs/pyGSTi/blob/0299e1d13222658ab385a17f6e16b9c7ce566163/pygsti/algorithms/gaugeopt.py#L636-L640

The documentation of entanglement_fidelity says its arguments need to be numpy arrays. However, the code above passes in pyGSTi modelmember objects of one kind or another. If you take a quick look at the implementation of entanglement_fidelity then you'll find that this can easily result in an error. (For example, if either operation is a CompsedOp object.)

This specific issue could be fixed by replacing 638 -- 640 in the gaugeopt.py snippet to the following:

op_a = target_model.operations[opLbl].to_dense()
op_b = mdl.operations[opLbl].to_dense()
infidelity = 1.0 - _tools.entanglement_fidelity(op_a, op_b, mxBasis)
ret += wt*(infidelity ** 2)

That said, the issue isn't isolated to the snippet above. Similar bugs exist throughout the definition of the the objective function for non-LS gauge optimization models.

coreyostrove commented 6 months ago

Great job catching this, @rileyjmurray!

I'm not positive what the history is behind this, but I suspect this is vestigial from the time before more complicated modelmembers for operations existed when pretty much all of the operations inherited from DenseOperatorInterface or similar, and as such behaved in many contexts as if they were numpy arrays. This is clearly no longer true, and your proposed solution makes sense to me.

enielse commented 6 months ago

I agree with Corey's assessment - we haven't tried gauge optimizing to fidelity in a long time and the initial implementation was more experimental research and there are likely no unit tests. So yeah, converting operators to dense matrices first is I think what you want to do.

On Tue, Mar 19, 2024 at 3:24 PM coreyostrove @.***> wrote:

Great job catching this, @rileyjmurray https://github.com/rileyjmurray!

I'm not positive what the history is behind this, but I suspect this is vestigial from the time before more complicated modelmembers for operations existed when pretty much all of the operations inherited from DenseOperatorInterface or similar, and as such behaved in many contexts as if they were numpy arrays. This is clearly no longer true, and your proposed solution makes sense to me.

— Reply to this email directly, view it on GitHub https://github.com/sandialabs/pyGSTi/issues/406#issuecomment-2007956375, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACA7BCZ7WZLG34TKK2EXHRDYZCGHJAVCNFSM6AAAAABECNHCP6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMBXHE2TMMZXGU . You are receiving this because you are subscribed to this thread.Message ID: @.***>

sserita commented 6 months ago

I think this particular bug is basically solved by Timo's PR #414. By performing the to_dense cast inside entanglement_fidelity, we avoid having to find all the places where the function is called.

The only thing I haven't check as part of that is whether similar bugs exist for other optool functions, which I'll do before closing out this issue.

sserita commented 6 months ago

Closing as Timo's PR did in fact fix the other possible optool functions (average_gate_fidelity and unitarity).