Skip to content

Conversation

@95-martin-orion
Copy link
Collaborator

Resolves the "tests are failing" part of #5524.

@95-martin-orion 95-martin-orion requested review from a team, cduck and vtomole as code owners June 15, 2022 16:40
@95-martin-orion 95-martin-orion requested a review from maffoo June 15, 2022 16:40
@CirqBot CirqBot added the size: S 10< lines changed <50 label Jun 15, 2022
Copy link
Collaborator

@viathor viathor left a comment

Choose a reason for hiding this comment

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

AFAIR, this is the first time this happened with quimb. I'd consider pinning if it keeps happening and affects us more seriously.

@maffoo
Copy link
Contributor

maffoo commented Jun 15, 2022

Thanks for looking into this, @95-martin-orion.

This change will cause tests to fail for people who have older versions of quimb installed locally, which seems less than ideal. I don't think we should be relying on string representations provided by third-party libraries to be stable in our tests.

@95-martin-orion
Copy link
Collaborator Author

This change will cause tests to fail for people who have older versions of quimb installed locally, which seems less than ideal.

This isn't the first time we've done something like this - the usual solution is to announce the change and recommend re-installing Cirq requirements.

I don't think we should be relying on string representations provided by third-party libraries to be stable in our tests.

This is the cost of allowing MPS to use quimb-based state for its string representation. Some alternatives:

  • Manually construct a string for the quimb state in Cirq
  • Don't reveal the state in the string representation
  • Don't test the string representation (likely fails coverage checks)

@maffoo, what are your thoughts? I'll hold off on merging until I have your approval.

@maffoo
Copy link
Contributor

maffoo commented Jun 15, 2022

My preference in order:

  1. Don't test the string representation (or at least, don't test for exact equality, just test that str(...) works and maybe that it contains info that we control and care about)
  2. Don't reveal the state in the string representation (seems less useful for users)
  3. Manually construct a string for the quimb state in Cirq (seems like work we shouldn't need to take on)

Copy link
Contributor

@maffoo maffoo left a comment

Choose a reason for hiding this comment

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

Approving so we can merge to unblock CI. We can discuss changes to the tests to avoid relying on stability of quimb strings in the future.

@95-martin-orion 95-martin-orion merged commit c504c38 into master Jun 15, 2022
@95-martin-orion 95-martin-orion deleted the quimb-1_4_0-sync branch June 15, 2022 18:49
CirqBot pushed a commit that referenced this pull request Jun 15, 2022
Fixes #5524.

Based on #5525 discussion, we don't need to pin quimb (yet!), but testing exact strings for quimb objects is asking for trouble. This reduces the tests to substring checks, which should be less vulnerable to minor quimb movements (e.g. this passes with both quimb v1.3.0 and v1.4.0).
rht pushed a commit to rht/Cirq that referenced this pull request May 1, 2023
rht pushed a commit to rht/Cirq that referenced this pull request May 1, 2023
Fixes quantumlib#5524.

Based on quantumlib#5525 discussion, we don't need to pin quimb (yet!), but testing exact strings for quimb objects is asking for trouble. This reduces the tests to substring checks, which should be less vulnerable to minor quimb movements (e.g. this passes with both quimb v1.3.0 and v1.4.0).
harry-phasecraft pushed a commit to PhaseCraft/Cirq that referenced this pull request Oct 31, 2024
harry-phasecraft pushed a commit to PhaseCraft/Cirq that referenced this pull request Oct 31, 2024
Fixes quantumlib#5524.

Based on quantumlib#5525 discussion, we don't need to pin quimb (yet!), but testing exact strings for quimb objects is asking for trouble. This reduces the tests to substring checks, which should be less vulnerable to minor quimb movements (e.g. this passes with both quimb v1.3.0 and v1.4.0).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size: S 10< lines changed <50

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants