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

Feature/pauli string test #10

Merged
merged 11 commits into from
Aug 5, 2024
Merged

Feature/pauli string test #10

merged 11 commits into from
Aug 5, 2024

Conversation

stand-by
Copy link
Collaborator

PR with test coverage around python bindings

Copy link
Contributor

@jamesETsmith jamesETsmith left a comment

Choose a reason for hiding this comment

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

A couple of things here:

  • The directory names approvals and bindings are a bit ambiguous. I've spent the past few minutes pondering how to break up these different classes of bindings and can't come up with anything I'm excited about. The best I can come up with is cpp, py, and py_cpp, where those directories test the cpp code for correctness, the python code for correctness, and consistency between the cpp and python versions. Even as I write it, that sounds like a lot of duplicate code, but let's talk offline about a way to reuse as much code as possible.
  • There are some tests with three or four analogous tests with different hard coded numbers, I think we can reduce the LOC in those tests by using pytest.mark.parameterize.

fast_pauli/cpp/include/__pauli_helpers.hpp Outdated Show resolved Hide resolved
fast_pauli/cpp/src/fast_pauli.cpp Outdated Show resolved Hide resolved
fast_pauli/cpp/include/__pauli_string.hpp Show resolved Hide resolved
tests/approvals/test_pauli_string.py Show resolved Hide resolved
tests/approvals/test_pauli_string.py Outdated Show resolved Hide resolved
tests/bindings/test_pauli.py Outdated Show resolved Hide resolved
tests/bindings/test_pauli.py Outdated Show resolved Hide resolved
@jamesETsmith
Copy link
Contributor

Also let's update the CI to run all these tests (through make)

@stand-by
Copy link
Collaborator Author

stand-by commented Aug 1, 2024

A couple of things here:

  • The directory names approvals and bindings are a bit ambiguous. I've spent the past few minutes pondering how to break up these different classes of bindings and can't come up with anything I'm excited about. The best I can come up with is cpp, py, and py_cpp, where those directories test the cpp code for correctness, the python code for correctness, and consistency between the cpp and python versions. Even as I write it, that sounds like a lot of duplicate code, but let's talk offline about a way to reuse as much code as possible.
  • There are some tests with three or four analogous tests with different hard coded numbers, I think we can reduce the LOC in those tests by using pytest.mark.parameterize.

I agree on a lot of points from your comment. I'll do a pass over tests and try to parametrize quick&easy cases. Also see my comment highlighting similar concerns.
As discussed, I've put together an asana ticket with desired test structure that we should benefit from and have as little code duplication as possible

Copy link
Contributor

@jamesETsmith jamesETsmith left a comment

Choose a reason for hiding this comment

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

LGTM @stand-by. I added an Asana ticket to track the unification of the test infrastructure here

@stand-by stand-by merged commit fae3020 into develop Aug 5, 2024
3 checks passed
@stand-by stand-by deleted the feature/pauli_string_test branch August 5, 2024 17:02
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