Better API for choosing a decoder

[perlinm]

See comment by @beitom

[perlinm]

Pulling in #409 into scope here: maybe we should use a DecodingOptions container instead of accepting **decoding_args everywhere.

[perlinm]

Current proposal: every method that currently takes a **decoding_args should instead take two inputs:

1. A DecoderName enum type to specify the decoder, and
2. A DecodingOptions object that stores all options to pass to the decoder specified by the DecoderName.

[perlinm]

Maybe qLDPC should also rename Decoder -> SyndromeDecoder and SinterDecoder -> ObservableDecoder for clarity.

[HaoTy]

I think the current functional API has its advantages - it uses built-in types and doesn't require the user to import any custom class (we all like numpy but not qiskit). But the problem with **kwargs is that it hides away the available options. Specifically in our case of trying to use SlidingWindowDecoder, it was not obvious to me that this was the place to pass in the actual decoder. Now with your clarification, I see what the docstring means. My intuition was to find an argument that took a Decoder class, so I'd say this is the API that makes the most sense to me. Why bother doing

1. A DecoderName enum type to specify the decoder, and
2. A DecodingOptions object that stores all options to pass to the decoder specified by the DecoderName.

when the decoders already have their own classes? Maybe I'm not familiar enough with the code base and am not seeing the big picture here.

[perlinm]

I guess this is a question for the user (you): how could the API for constructing decoders be more intuitive and/or clear? What would it take for a user to (quickly) figure out what they need without consulting me?

An obvious question is better documentation. But then

1. What exactly should be documented where (in a maintainable way, e.g. without copying variations of the same blurb in 15 different places)? And
2. Is more documentation enough? I am open to either answer here, but I suspect that we could build a more intuitive API.

An example notebook about choosing decoders would probably also help.

[beitom]

A suggested API to add to the discussion:

Replace all the current kwargs like with_BP_OSD=True, with_MWPM=True, decoder_constructor=..., static_decoder=..., and **decoder_kwargs with named helper functions that user can discover from qldpc.decoders autocomplete and will have typed args.

decoders.bp_osd(...)
decoders.bp_lsd(...)
decoders.bf(...)
decoders.mwpm(...)

Some examples

decoders.decode(H, s, decoder=decoders.mwpm())
code.get_logical_error_rate_func(..., decoder=decoders.bp_lsd(max_iter=30, bp_method="ms"))
decoders.SlidingWindowDecoder(window_size=5, stride=2, decoder=decoders.bp_lsd(...))
css_code.get_logical_error_rate_func(..., decoder_x=decoders.bp_lsd(...), decoder_z=decoders.mwpm())

There are many ways to implement this, one possiblity is to make these helpers return an internal dataclass with a 1:1 copy of their parameters that gets registered and called by an internal builder. This is not super clean and it would good to review by someone with better functional programming knoweldge.

def _spec(id: DecoderId, **options: Any) -> DecoderSpec:
    return DecoderSpec(id=id, options=MappingProxyType(options))

def bp_lsd(
    *,
    error_rate: float = PLACEHOLDER_ERROR_RATE,
    error_channel: npt.NDArray[np.float64] | Sequence[float] | None = None,
    max_iter: int = 30,
    bp_method: Literal["ms", "ps"] = "ms",
    lsd_method: str = "lsd_cs",
    lsd_order: int = 0,
) -> DecoderSpec:
    return _spec(
        "bp_lsd",
        error_rate=error_rate,
        error_channel=error_channel,
        max_iter=max_iter,
        bp_method=bp_method,
        lsd_method=lsd_method,
        lsd_order=lsd_order,
    )

# callers like decode use a single resolve method
def decode(pcm_or_dem: Any, syndrome: Any, *, decoder: DecoderSpec = None) -> Any:
    return resolve_decoder(pcm_or_dem, decoder).decode(syndrome)

# resolve looks up in a registry
DecoderInput = Decoder | DecoderSpec | None

def resolve_decoder(pcm_or_dem: Any, decoder: DecoderSpec) -> Decoder:
     # could also handle default logic if no decoder is specified
     factory = _REGISTRY[decoder.id] 
     return factory.build(pcm_or_dem, decoder)

# decoder builders could be registered automatically with a decorator that adds them to _REGISTRY
@register_decoder("mwpm", priors_arg="weights", log_likelihood_priors=True, summary="PyMatching MWPM")
def _build_mwpm(pcm_or_dem: Any, spec: DecoderSpec) -> Decoder:
    return _build_mwpm_decoder(pcm_or_dem, **dict(spec.options))

A more object oriented option that would be much cleaner to implement but may feel less fun to the user

# public type used via decoders.BPLSD(param)
# build is called internally by decoders.decode etc
@dataclass(frozen=True, slots=True, kw_only=True)
class BPLSD:
    priors_arg: ClassVar[str | None] = "error_channel"
    log_likelihood_priors: ClassVar[bool] = False

    error_rate: float = ...
    error_channel: npt.NDArray[np.float64] | Sequence[float] | None = None
    ...


    def build(self, pcm_or_dem: PcmOrDem) -> Decoder:
        pcm, error_channel = _to_ldpc_inputs(pcm_or_dem, self.error_rate, self.error_channel)
        return ldpc.bplsd_decoder.BpLsdDecoder(
            pcm,
            error_channel=error_channel,
            ...
        )

# used to get decoders by internal tools like decode
# types the decoder with a `DecoderBuilder` which could be a plain Protocol/abstract with a build method
def resolve_decoder(pcm_or_dem: Any, decoder: DecoderBuilder) -> Decoder:
     # could also handle default logic for None
     return decoder.build(pcm_or_dem)

[HaoTy]

I see, so the reason the user can't directly pass in a Decoder instance is that it requires pcm_or_dem, which is not known. Then both proposals make sense. Maybe we can have both, so the user has a choice? If the object-oriented approach is adopted, the current decoder classes should be less accessible to users (e.g., add a leading underscore) to avoid confusion.

The way of invoking a SinterDecoder should be consistent with whatever approach we adopt, but at the same time the two types of decoders need to be clearly distinguishable...

Documentation certainly helps. Object-oriented with type hinting itself is good documentation and helps deduplicate the same docstrings. Once the argument says it accepts a, e.g., DecoderSpec, the user will look for information there. Btw, the doc page link in README doesn't work, not sure if it's intended (from the context, it might be, but there's a .readthedocs.yaml).

[beitom]

I see, so the reason the user can't directly pass in a Decoder instance is that it requires pcm_or_dem, which is not known. Then both proposals make sense. Maybe we can have both, so the user has a choice? If the object-oriented approach is adopted, the current decoder classes should be less accessible to users (e.g., add a leading underscore) to avoid confusion.

Yup this is it, for example LookupDecoder

qLDPC/src/qldpc/decoders/custom.py

Line 270 in 2f7af50

for error, syndrome in LookupDecoder.iter_errors_and_syndomes(pcm, max_weight, symplectic):

will prelloacte the lookup table from the PCM in the constructor and ldpc bp decoder https://github.com/quantumgizmos/ldpc/blob/main/src_cpp/bp.hpp allocates several reusable buffers/build tanner graph structure

The way of invoking a SinterDecoder should be consistent with whatever approach we adopt, but at the same time the two types of decoders need to be clearly distinguishable...

Agreed, SinterDecoder should also take a decoder type as argument in

qLDPC/src/qldpc/decoders/sinter.py

Line 46 in 2f7af50

**decoder_kwargs: object,

e.g.

decoders.SlidingWindowDecoder(
    window_size=5,
    stride=2,
    decoder=decoders.BDLSD(max_iter=30, bp_method="ms"),
)

Documentation certainly helps. Object-oriented with type hinting itself is good documentation and helps deduplicate the same docstrings. Once the argument says it accepts a, e.g., DecoderSpec, the user will look for information there. Btw, the doc page link in README doesn't work, not sure if it's intended (from the context, it might be, but there's a .readthedocs.yaml).

Inline docs are the best docs :)

[beitom]

@perlinm unrelated to this discussion but while reviewing the decoders I came across a potential bug with

qLDPC/src/qldpc/decoders/sinter.py

Line 674 in 2f7af50

while start_time < num_time_points - max_size_of_last_window:

It looks like start_time = min(time_to_dets) an absolute time is being compared to num_time_points = max(time_to_dets) - start_time + 1 a relative time which would lead to a big if start_time > 0.

If I'm not missing some important context it should be as simple as changing to

start_time = min(time_to_dets)
end_time = max(time_to_dets) + 1
max_size_of_last_window = self.window_size + self.stride - 1

while start_time < end_time - max_size_of_last_window:
   ...

pushed a fix here main...beitom:qLDPC:SlidingWindowDecoder-bug-fix will send a PR if this is a real issue.

[perlinm]

@beitom that indeed looks like a bug and the correct fix, thank you!

Also num_time_points in

qLDPC/src/qldpc/decoders/sinter.py

Line 686 in 2f7af50

window_time_to_dets = [time_to_dets[tt] for tt in range(start_time, num_time_points)]

should be end_time.