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

Add cirq.testing.assert_same_diagram #830

Closed
Strilanc opened this issue Aug 8, 2018 · 9 comments
Closed

Add cirq.testing.assert_same_diagram #830

Strilanc opened this issue Aug 8, 2018 · 9 comments
Labels
good first issue This issue can be resolved by someone who is not familiar with the codebase. A good starting issue.

Comments

@Strilanc
Copy link
Contributor

Strilanc commented Aug 8, 2018

This method would do two things:

  1. Strip the diagrams for you, so you didn't have to worry about trailing newlines in the code.
  2. Give better error messages than python's default string error message.

For point 2, the main thing that is required is to append the actual diagram and reference diagram without abbreviation. Pytest is far too abbreviation heavy for the diagrams.

Secondarily for point 2, a better diff should be included. The information is 2d instead of line-based, and the diff should be similarly 2d. Show a copy of the actual diagram with any character positions that differ from the reference diagram replaced by ■ or some other obtrusive char.

@Strilanc Strilanc added the good first issue This issue can be resolved by someone who is not familiar with the codebase. A good starting issue. label Aug 8, 2018
@kevinsung
Copy link
Collaborator

For use cases I have encountered, I often want to ignore differences due to insertion strategies, i.e., I'd want to shove all gates to the left before comparing.

@Strilanc
Copy link
Contributor Author

Strilanc commented Aug 8, 2018

That would be better described as a circuit equivalence rather than a diagram equivalence.

@bryano
Copy link
Contributor

bryano commented Aug 9, 2018

CircuitDag would be good for that.

@cduck
Copy link
Collaborator

cduck commented Aug 10, 2018

Yes, except CircuitDag doesn't implement equality yet.

@Strilanc
Copy link
Contributor Author

Does the library we use have a graph isomorphism method?

@cduck
Copy link
Collaborator

cduck commented Aug 10, 2018

Probably, but it would have to support customizing node equality. The matching in the two graphs wouldn't compare equal by default.

@cduck
Copy link
Collaborator

cduck commented Aug 10, 2018

You can do it with is_isomorphic.

@Strilanc
Copy link
Contributor Author

Strilanc commented Oct 2, 2018

@dlyongemallo thanks for doing this, it is awesome how clear it is.

image

@Strilanc
Copy link
Contributor Author

Strilanc commented Oct 2, 2018

At some point we might want a better edit distance metric when making the difs, so that the later parts aren't so noisy, but this works really well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue This issue can be resolved by someone who is not familiar with the codebase. A good starting issue.
Projects
None yet
Development

No branches or pull requests

4 participants