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

Refactor PauliStrings as templates for easier interop #1081

Merged
merged 34 commits into from
Oct 31, 2023

Conversation

willsimmons1465
Copy link
Contributor

Across tket, we use QubitPauliString, PauliStabiliser, QubitPauliTensor, std::vector, std::pair<QubitPauliString, Expr> all to refer to some combination of a Pauli string (sparse or dense) and (optional coefficient). Reviewer feedback in #941 raised the point we should have one which restricts to Clifford phases {+1, +i, -1, -i} in place of QubitPauliTensor for Clifford and stabiliser code. This is all a bit much to manage separately and could do with a refactor for better code reuse, flexibility of structures, and standard conversions between them.

Class inheritance wasn't a great option because we have variations in both the string (sparse vs dense) and coefficient (none, Clifford, complex, symbolic), so I couldn't see an obviously clear hierarchy. Templates allow this multi-dimensional parameterisation easier.

I have refactored the code base to switch out the old with the new as best as I could determine. Json serialisation and a lot of the binder code is a bit ugly in order to leave no breaking changes to pytket users.

Style-wise, let me know if you have a better suggestion for naming conventions for the different typedef specialisations of PauliTensor. Also, I haven't changed variable names throughout (many still called e.g. qps or qpt in reference to old class names) so let me know if you want these updated.

@trvto
Copy link
Contributor

trvto commented Oct 16, 2023

The error at stub generation is occurring because SymPauliTensor is not bound to a python class, but is being used as part of the python interface to PauliExpCommutingSetBox. So you will need to define py::class_<SymPauliTensor>(... for it to work or find a way around having SymPauliTensor in the interface. See pytket/stub_generation/README.md for more info on how to re-generate the stubs locally. (You'll need to do that and commit the changes for this PR)

"string", &QubitPauliTensor::string,
.def(
"__mul__",
[](const Complex &c, const SpCxPauliTensor &qpt) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
[](const Complex &c, const SpCxPauliTensor &qpt) {
[](const SpCxPauliTensor &qpt, const Complex &c) {

With .def you define a normal method called on class objects and the first parameter is always "self".

.def_readwrite(
"string", &QubitPauliTensor::string,
.def(
"__mul__",
Copy link
Contributor

Choose a reason for hiding this comment

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

The old code .def(Complex() * py::self) caused this to be defined as __rmul__. I'm not sure if it matters but maybe best to use __rmul__ here as well

@willsimmons1465
Copy link
Contributor Author

Thanks for the pointer. I think I've now fixed the stubs.

I'll get to work on filling out the test coverage next week.

Copy link
Contributor

@yao-cqc yao-cqc left a comment

Choose a reason for hiding this comment

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

Good work! Only some minor comments

tket/src/PauliGraph/ConjugatePauliFunctions.cpp Outdated Show resolved Hide resolved
tket/include/tket/Utils/PauliTensor.hpp Show resolved Hide resolved
tket/src/Circuit/Boxes.cpp Show resolved Hide resolved
tket/include/tket/Utils/PauliTensor.hpp Show resolved Hide resolved
@willsimmons1465
Copy link
Contributor Author

Not sure how well gcovr works with templates. When I run it locally, it will only list functions for one or two specialisations and there are many of them that it claims are never called despite having tests that call them explicitly. For example, for the casting between PauliTensor variants, it only lists seven options, six of which are casts between different coefficients for sparse pauli strings. It claims none of these are ever called, but I have a test which very explicitly goes through every combination of casts between different coefficients for sparse strings.

If this still fails coverage, I might just have to add some tests for other parts of the codebase to pad out the stats and get the PR in.

@yao-cqc
Copy link
Contributor

yao-cqc commented Oct 31, 2023

Not sure how well gcovr works with templates. When I run it locally, it will only list functions for one or two specialisations and there are many of them that it claims are never called despite having tests that call them explicitly. For example, for the casting between PauliTensor variants, it only lists seven options, six of which are casts between different coefficients for sparse pauli strings. It claims none of these are ever called, but I have a test which very explicitly goes through every combination of casts between different coefficients for sparse strings.

If this still fails coverage, I might just have to add some tests for other parts of the codebase to pad out the stats and get the PR in.

@cqc-alec might be able to override the checks

@cqc-alec
Copy link
Collaborator

The coverage checker is unreliable with anything implemented in header files (including templates). Happy to merge this.

@cqc-alec cqc-alec merged commit fc43ebf into develop Oct 31, 2023
29 of 30 checks passed
@cqc-alec cqc-alec deleted the refactor/paulistring branch October 31, 2023 13:22
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.

4 participants