remove boilerplate in LDPCDecodersExt and PyQDecodersExt
#452
Conversation
There was a problem hiding this comment.
Thank you for looking into this -- there is indeed a lot of boilerplate that needs to be cleaned up here, and there are a lot of good changes in the PR you are proposing.
However, I am reluctant to do anything in this direction before there are more decoders being implemented. Early abstraction is pretty dangerous because it can then constrain what you can do in the future. There is this design rule of thumb to not introduce abstraction until there is sufficient amount of code repetition. Right now I still feel we are more on the edge of that threshold (admittedly that is a subjective opinion).
I left a few comments with more detailed review.
Another minor concern: this PR will probably break a lot of the label-generation functionality in the ecc wiki https://github.com/QuantumSavory/qECCBenchWiki (although admittedly, it is the label-making functionality that is done in a silly way, so it should be fixed anyway).
Given these considerations, I would prefer to close this PR with a label to reconsider it in the future.
| bpdecoderx | ||
| bpdecoderz | ||
| end | ||
| abstract type AbstractLDPCDecoder <: AbstractSyndromeDecoder end |
There was a problem hiding this comment.
I do not think this extra abstract type is necessary.
|
|
||
| struct BitFlipDecoder <: AbstractSyndromeDecoder # TODO all these decoders have the same fields, maybe we can factor out a common type | ||
| # A common structure to hold shared fields for different decoders | ||
| mutable struct GenericLDPCDecoder{D} <: AbstractLDPCDecoder |
There was a problem hiding this comment.
The name does not seem very appropriate to me. These are not that general, although they are a common way to do things. The basic feature here is that the decoders are built out of two classical decoders. Maybe a name like CSSDecoderFromClassical or IndependentCSSDecoders or PairOfClassicalDecoders or PairedClassicalDecoder or CSSClassicalPairDecoders. I do not really have a good suggestion right now.
| end | ||
|
|
||
| function BeliefPropDecoder(c; errorrate=nothing, maxiter=nothing) | ||
| # Common constructor for any LDPC decoder (both BitFlip and BeliefProp) | ||
| function GenericLDPCDecoder(c, DecoderType; errorrate=nothing, maxiter=nothing) |
There was a problem hiding this comment.
This function forces a very specific form that is expected from the decoders. I am not sure that form will be true for the next decoder to be added. This is one of my main worries with this PR
| @@ -8,89 +8,53 @@ import QuantumClifford.ECC: AbstractSyndromeDecoder, decode, batchdecode, parity | |||
|
|
|||
| abstract type PyBP <: AbstractSyndromeDecoder end | |||
|
|
|||
| struct PyBeliefPropDecoder <: PyBP # TODO all these decoders have the same fields, maybe we can factor out a common type | |||
| # A common structure to hold shared fields for different decoders | |||
| mutable struct GenericPyLDPCDecoder <: PyBP | |||
There was a problem hiding this comment.
the fact that we need two different Generic... structs makes me a bit uncomfortable with this approach. They can not be that generic if we need two versions of it. This is my second main concern with the design in this PR.
| if decoder_type == :beliefprop | ||
| pyx = ldpc.bp_decoder(np.array(Hx); max_iter, bp_method, error_rate) # TODO keep these sparse | ||
| pyz = ldpc.bp_decoder(np.array(Hz); max_iter, bp_method, error_rate) # TODO keep these sparse | ||
| elseif decoder_type == :beliefprop_os | ||
| pyx = ldpc.bposd_decoder(np.array(Hx); max_iter, bp_method, error_rate, osd_method, osdorder) | ||
| pyz = ldpc.bposd_decoder(np.array(Hz); max_iter, bp_method, error_rate, osd_method, osdorder) | ||
| else | ||
| error("Unknown decoder type.") | ||
| end |
There was a problem hiding this comment.
This is also a major concern. The previous approach was extendable. This if-else chain is not. Adding a new decoder would require a modification of this function (so it can not be done in an independent package), which is the main problem that Julia's multiple dispatch is meant to solve.
|
Thank you. I will close this in light of your helpful feedback. Please add the appropriate label! |
Krastanov
added
the
to be revived
This PR aims to remove the boilerplace/common logic that is present in both LDPCDecodersExt and PyQDecodersExt.
This is the task that was highlighted in the TODOs of these extensions. I have ran the tests on Julia 1.11.1, not Julia 1.11.2 as I have not updated Julia version yet.