Skip to content

Conversation

@akirakyle
Copy link
Member

Applies on top of #41. Closes #36. See #39 for more discussion.

I think eventual removal of PauliBasis rather than deprecation with renaming makes the most sense. This is because it is identical to SpinBasis(1//2)^N, however with the sever downside that it is not compatible with any of the CompositeBasis machinery such as reduced, ptrace, embed, permutesystems, etc...

@akirakyle akirakyle requested a review from Krastanov April 6, 2025 18:30
@akirakyle akirakyle changed the title Depricate pauli Depricate PauliBasis Apr 6, 2025
@codecov
Copy link

codecov bot commented Apr 6, 2025

Codecov Report

Attention: Patch coverage is 0% with 6 lines in your changes missing coverage. Please review.

Project coverage is 34.90%. Comparing base (15479d1) to head (3f46199).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/deprecated.jl 0.00% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #52      +/-   ##
==========================================
+ Coverage   34.75%   34.90%   +0.15%     
==========================================
  Files          15       15              
  Lines         423      424       +1     
==========================================
+ Hits          147      148       +1     
  Misses        276      276              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@akirakyle akirakyle changed the title Depricate PauliBasis Deprecate PauliBasis Apr 7, 2025
@Krastanov
Copy link
Member

let's change the phrasing a bit. PauliBasis will never be used as a name again, this is just asking for trouble.

How about we just rename it to QubitBasis? And then add Base.@deprecate_binding PauliBasis QubitBasis

As an added bonus, this would make the change drastically smaller, with little to no code moved around.

Copy link
Member

@Krastanov Krastanov left a comment

Choose a reason for hiding this comment

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

left a comment with a suggested simplification for this. I prefer using the standard deprecation macros instead of custom messages.

If we are deprecating this name, we should not use it again in the future for something else. That would just cause problems.

Marking as draft to keep my review queue manageable. Please mark back as ready whenever convenient.

@Krastanov Krastanov marked this pull request as draft April 20, 2025 00:21
@akirakyle
Copy link
Member Author

akirakyle commented Apr 20, 2025

Hm I'm not sure I like the implication of never being able to use the name PauliBasis again. The whole reason I wanted to do this was so that the name PauliBasis could continue to be used as part of the Pauli transfer matrix and chi matrix implementations in QuantumOpticsBase, but with the correct meaning as the actual Pauli basis. The only usage of PauliBasis in qojulia packages is in the tests for QuantumOpticsBase.jl/src/pauli.jl.

So perhaps a different path forward would be to just not deprecate, but change it's struct members as necessary for changes in the superoperator code which unify superoperators and pauli transfer matrices given they're simply related by a basis change? I already have this working in #55 and qojulia/QuantumOpticsBase.jl#196

@Krastanov
Copy link
Member

I agree that it is misleading to call this basis a "Pauli Basis", but that mistake was made and we should not break user code that relies on the mistake. The deprecation is to avoid future user code to use something with a confusing name. While it would be much neater to get to use this name for something for which it actually applies, the cost in terms of future confusion is just too great. If you need something to represent a basis of the space of operators, we can use a name like PauliOpBasis. Either way, having an Op or something similar in the name would be helpful as otherwise the name would be confusingly similar to the names for state space bases.

@akirakyle
Copy link
Member Author

I agree that it is misleading to call this basis a "Pauli Basis", but that mistake was made and we should not break user code that relies on the mistake.

I would be very surprised if there is code that uses PauliBasis since I can't imagine how it would be used over SpinBasis(1//2)^N given that

  1. It is not compatible with any of the CompositeBasis machinery such as reduced, ptrace, embed, permutesystems, etc..
  2. It isn't even used in the pauli transfer matrix code where one would expect it would help in extracting the relevant coefficients of a channel for a given pauli string.

But I suppose this maybe irrelevant if the goal is to hang onto this basis in some form and the constraint is that we cannot use this name for an operator basis.

we can use a name like PauliOpBasis

That is okay with me for the operator basis.

How about we just rename it to QubitBasis?

I think we should decide on what "the canonical qubit basis" should be. The current de facto basis seems to be SpinBasis(1//2). NLevelBasis(2) also would be perfectly fine. QubitBasis would certainly be the clearest "canonical qubit basis".

Deciding this is necessary for the new basis and superoperator interface and subsequent improved implementations of the Pauli transfer matrix and chi matrix because

  1. When converting from PTM/chi back to superoperator, a choice of basis for the superoperator needs to be made.
  2. Currently there is no basis checking, only implicit checking by matrix multiplication if the dimension is a power two. So GenericBasis(2)⊗SpinBasis(1//2)⊗NLevelBasis(2)⊗FockBasis(1) is currently considered fine as a qubit basis whereas this should be an error with a message saying what basis is correct.

@akirakyle
Copy link
Member Author

Hmmm on further though I'm actually not sure a separate QubitBasis is a good idea. QuantumOpticsBase would then need to duplicate methods for sigmax/paulix for QubitBasis etc. An advantage of designating NLevelBasis(2) as the canonical qubit basis is that the clock and shift matrix generalization of the pauli operators to qubits then immediately generalizes the pauli transfer matrix to qudits since they form an operator basis. This is actually something I was planning to implement as part of the superoperator stuff.

@akirakyle akirakyle marked this pull request as ready for review April 22, 2025 00:39
@Krastanov
Copy link
Member

Your reasoning sounds good to me, at least to the extent I follow.

If PauliBasis indeed does not provide anything more than SpinBasis(1//2), let's just continue with the deprecation you started and for now let's NOT introduce QubitBasis as an alternative name. If we need an operator basis object, let's use a different name, e.g. PauliOpBasis.

If one day someone explains why PauliBasis is useful to them, we can introduce a new, non-deprecated name or add the necessary features to some appropriate composite basis or SpinBasis(1//2) or NLevelBasis(2).

Q1: The PR that introduced PauliBasis introduced a bunch more capabilities that seem to refer to PauliBasis. Are these capabilities things that you are updating/redesigning in the rest of your work (so that they do not depend on a deprecated basis object)? Here is that PR https://github.com/qojulia/QuantumOptics.jl/pull/251/files

Q2: Could you double check the deprecation message -- it feels like there is a word or subclause missing from it?

Feel free to merge this and the related PR and do version bumps and releases after this is done. Just make sure tests are passing and that we indeed just have deprecations, not breakages.

@akirakyle
Copy link
Member Author

Q1: The PR that introduced PauliBasis introduced a bunch more capabilities that seem to refer to PauliBasis. Are these capabilities things that you are updating/redesigning in the rest of your work (so that they do not depend on a deprecated basis object)? Here is that PR https://github.com/qojulia/QuantumOptics.jl/pull/251/files

Yes the Pauli transfer matrix (PTM) and Chi process matrix which it introduced are things I'm working on updating with the new basis interface and have been helpful in guiding what that looks like. Since the PTM and Chi matrix are just the superoperator and choi state in the Pauli operator basis respectively, it makes sense for them to share common code (and this also gives algorithmic speedups from the current implementations).

But this leads me to a question. Originally PauliBasis was introduced in that PR as a way to enforce a uniform basis for the PTM and Chi matrices, but was subsequently loosened in this PR: qojulia/QuantumOpticsBase.jl#28 which removed all uses of PauliBasis, except in test files along with removing type restrictions on parameters in many other types. I couldn't figure out what the reason for this was so perhaps you or @david-pl could say? This also has relevance to the basis interface work. For example in #34 there was discussion about changing the .basis field of CompositeBasis from a tuple to vector. I implemented that in #55 and also constrained the type parameter of the field to Vector{B} where B<:Basis. I thought it would be better to enforce that rather than have potentially buggy behavior elsewhere due to it accidentally ending up as a Tuple instead. But the PR loosening type constraints make me think there's maybe some reason to prefer leaving them unconstrained?

Q2: Could you double check the deprecation message -- it feels like there is a word or subclause missing from it?

Oops, fixed now. Unless there's anything else, I'll merge and release tomorrow!

@Krastanov
Copy link
Member

I thought it would be better to enforce that rather than have potentially buggy behavior elsewhere

yeah... My take is that you are describing a philosophical conundrum at the core of Julia dev style and at the core of the "expression problem". What follows is a personal opinion:

Types in function signatures in julia are to manage dispatch, not to ensure correctness. Even typeasserts in function bodies are there just to help the compiler, not to ensure correctness. Julia types do not let you specify an "interface", e.g. you might very well have something that behaves like an integer (it informally implements the Integer interface/contract), but it does not subtype Integer. Julia types might let you specify traits, which can provide some similar capabilities valuable for correctness checking, but that is a manual process not well supported by dev tooling and linters.

Thus, relaxing types in function signatures, and relaxing type constraints in structs (while keeping the structs concrete, just more highly parameterized) enables much higher interoperability between packages. Because the Julia compiler will specialize all methods anyway, you do not lose on performance.

But indeed, this higher "interoperability" thanks to lessened type constraints, leads to fewer "correctness constraints promised by the compiler". I claim these correctness constraints would have been bad anyway, because they should be based on interfaces (which julia does not support yet).

An example where such a "correctness constraint" was very annoying for me and stopped me from implementing a cool feature in a symbolic-numeric monte-carlo simulator: JuliaCollections/DataStructures.jl#860 -- removing this constraint made this possible in a symbolic manner: https://qc.quantumsavory.org/dev/noisycircuits_perturb/

Some suggestions for potential implementation of interfaces in julia: https://hackmd.io/BbEw0_B4Q8uDSS34LOvpCw

Anyway, my rant does not really change your plans, but hopefully it explains the philosophy behind the changes you referenced. For the moment please stick to your plan to be strict with type constraints -- we can relax things when necessary.

@akirakyle
Copy link
Member Author

Some suggestions for potential implementation of interfaces in julia: https://hackmd.io/BbEw0_B4Q8uDSS34LOvpCw

Thank you for that insightful comment and the interesting link! That definitely helps clarify the issue as a sort of tension between interoperability and typing checking arising from the lack of formal interfaces.

@akirakyle akirakyle merged commit f7aaafa into qojulia:main Apr 23, 2025
9 of 12 checks passed
@akirakyle akirakyle deleted the depricate-pauli branch April 23, 2025 01:07
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.

PauliBasis is misleadingly named as it doesn't represent the operator basis of Pauli matrices+identity

2 participants