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

Upgrade unittest equality method #1132

Merged

Conversation

nkanazawa1989
Copy link
Collaborator

Summary

Current implementation of equality check, i.e. QiskitExperimentsTestCase.json_equiv, is not readable and scalable because it implements equality check logic for different types in a single method. This PR adds new test module test/extended_equality.py which implements new equality check dispatcher is_equivalent.

Developers no longer need to specify check_func in the assertRoundTripSerializable and assertRoundTripPickle methods unless they define custom class for a specific unittest. This simplifies unittests and improves readability of equality check logic (and test becomes more trustable).

This PR adds new software dependency in develop; multimethod

Among several similar packages, this is chosen in favor of

  • its license type (Apache License, Version 2.0)
  • syntax compatibility with functools.singledispatch
  • support for subscripted generics in typings, e.g. Union

Copy link
Collaborator

@coruscating coruscating left a comment

Choose a reason for hiding this comment

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

This is a really nice upgrade and consolidation of the current comparison test code and a good use of multimethod. My comments are mainly about edge cases in the equality check.

test/extended_equality.py Outdated Show resolved Hide resolved
test/extended_equality.py Show resolved Hide resolved
@coruscating
Copy link
Collaborator

Thanks @nkanazawa1989, the fixes look good to me. I wonder if there should be an optional strict flag for is_equivalent() so that there is the option to not equate numpy arrays/tuples/lists with the same elements, or the numpy int and python int. Maybe there's no good use case for this though. Also, is your plan to wait for Qiskit/qiskit#9676 to be released before merging this?

@nkanazawa1989
Copy link
Collaborator Author

I wonder if there should be an optional strict flag

This sounds like a good idea. Done in 47dd5d5. I also added an option for numerical precision.

Also, is your plan to wait for Qiskit/qiskit#9676 to be released before merging this?

Yes. We can wait for 0.24rc1.

Copy link
Collaborator

@coruscating coruscating left a comment

Choose a reason for hiding this comment

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

This looks good now, thanks for adding strict_type. I'm good with merging once terra 0.24 is released and set as a requirement.

nkanazawa1989 and others added 4 commits May 12, 2023 03:14
- Cleanup doc
- Add handling for edge case

Co-authored-by: Helena Zhang <Helena.Zhang@ibm.com>
@nkanazawa1989 nkanazawa1989 added this pull request to the merge queue May 11, 2023
Merged via the queue into qiskit-community:main with commit 2278679 May 11, 2023
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