feat: add MLE (Gurobi) and MLD (Table) decoders with Sinter adapters#19
feat: add MLE (Gurobi) and MLD (Table) decoders with Sinter adapters#19ChenZhao44 wants to merge 21 commits intomainfrom
Conversation
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Tests for MLE decoder migrated from brute-force-decoders, importing from bloqade.decoders. All tests fail because GurobiDecoder doesn't exist yet. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…s (GREEN) Adds GurobiDecoder as a BaseDecoder subclass with lazy gurobipy/scipy imports. Includes decode_error, logical_from_error, weight_from_error, and generate_conditional_decoder methods. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Tests for MLD decoder and utility functions migrated from brute-force-decoders. All tests fail because TableDecoder doesn't exist yet. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… (GREEN) Adds TableDecoder as a BaseDecoder subclass with batch-optimized decode path. Includes utility functions pack_boolean_array, unpack_boolean_array, shots_to_counts, det_obs_shots_to_counts. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Tests for SinterGurobiDecoder and SinterTableDecoder migrated from brute-force-decoders. All tests fail because the sinter module doesn't exist yet. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
SinterGurobiDecoder lives in mle.py, SinterTableDecoder in mld.py. Removed standalone sinter.py. Each decoder file is self-contained with its own sinter wrapper. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Removed standalone test_sinter.py. SinterGurobiDecoder tests now live in test_mle.py, SinterTableDecoder tests in test_mld.py. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Move `import sinter` from module level to try/except guard for optional dependency isolation (issue #1) - Add `super().__init__(dem)` calls in GurobiDecoder and TableDecoder (issue #2) - Consolidate 3 DEM iteration passes into single pass in GurobiDecoder.__init__ (issue #3) - Replace assert with ValueError/RuntimeError for runtime validation in mld.py (issue #4) - Add @overload signatures for GurobiDecoder.decode() to clarify return type based on return_weights flag (issue #5) - Suppress Gurobi env log file by using gp.Env() without args (issue #6) - Replace print("Building decoder...") with logging.info (issue #7) - Move _check_no_separators to @staticmethod on GurobiDecoder (issue #8) - Extract duplicate test helpers to test/decoders/conftest.py (issue #9) - Add comment explaining TableDecoder exclusion from parametrized test suite (issue #10) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Remove `typing.override` (3.12+) from mld.py and mle.py for Python 3.10 support - Fix import ordering in __init__.py files to satisfy isort - Fix string formatting in conftest.py to satisfy black 26.x Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
☂️ Python Coverage
Overall Coverage
New Files
Modified Files
|
Collapse multiline string args to match black 26.x style. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add tests for: separator target rejection, multi-observable DEMs, repeat block DEMs, single-shot decode with weights, and validation error paths in update_det_obs_counts/decode_det_obs_counts. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds two new decoders (MLE via Gurobi MIP and MLD via lookup table) plus Sinter adapter classes, wires them into bloqade.decoders exports, and introduces optional dependency groups and tests to cover the new functionality.
Changes:
- Add
GurobiDecoder(MLE) +SinterGurobiDecoderadapter. - Add
TableDecoder(MLD) +SinterTableDecoderadapter. - Export new decoders and extend unit tests/import tests; add new optional dependency groups in
pyproject.toml.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
src/bloqade/decoders/_decoders/mle.py |
Implements Gurobi-based MLE decoder and Sinter adapter. |
src/bloqade/decoders/_decoders/mld.py |
Implements table-based MLD decoder and Sinter adapter. |
src/bloqade/decoders/_decoders/__init__.py |
Re-exports new decoders from internal package. |
src/bloqade/decoders/__init__.py |
Public API exports for the new decoders/adapters. |
pyproject.toml |
Adds optional dependency groups for MLE/MLD/Sinter and extends dev deps. |
test/test_imports.py |
Verifies new exports exist in bloqade.decoders. |
test/decoders/test_mle.py |
Unit tests for GurobiDecoder and SinterGurobiDecoder. |
test/decoders/test_mld.py |
Unit tests for TableDecoder and SinterTableDecoder. |
test/decoders/test_decoders.py |
Adds GurobiDecoder to the generic decoder test matrix. |
test/decoders/conftest.py |
Shared helpers for Sinter adapter tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Guard scipy import with ImportError and install hint - Init max_observable_index to -1 for zero-observable DEMs - Size observable_indices from dem.num_observables - Flatten DEM in __init__ and iterate over flattened version - Always set Gurobi OutputFlag explicitly (verbose or not) - Use GRB.OPTIMAL and raise RuntimeError on non-optimal status - Validate det_obs_counts shape in TableDecoder.__init__ - Fix Black formatting in __init__.py imports - Add tests for repeat block and nested repeat block flattening Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
| STEP_SIZE = 65536 |
There was a problem hiding this comment.
I'm not a big fan of having this global constant, can we just make it an argument to the constructor?
| try: | ||
| from sinter import ( | ||
| Decoder as _SinterDecoder, | ||
| CompiledDecoder as _SinterCompiledDecoder, | ||
| ) | ||
| except ImportError: | ||
| _SinterCompiledDecoder = object # type: ignore[assignment,misc] | ||
| _SinterDecoder = object # type: ignore[assignment,misc] |
There was a problem hiding this comment.
I don't think this is a very safe approach to handling the case where sinter is not found.
I think the best thing to do is to make a submodule like mld.sinter_interface which has this kind of try-except at the top level of the file. Then keep the core TableDecoder right where it is.
So if a user wants the default decoder just import bloqade.decoders.mld and if they want the sinter compatible version do bloqade.decoders.mld.sinter_interface. That's when this check can be triggered.
| @property | ||
| def is_cached_correction(self) -> bool: | ||
| return self._is_cached_correction | ||
|
|
||
| @property | ||
| def is_cached_df(self) -> bool: | ||
| return self._is_cached_df |
There was a problem hiding this comment.
Are some of these properties only used for testing? If that's the case I don't think it's worth having them and you can just directly access the properties you need.
If it is the case you expect other users to access these values outside of the object itself, then it's worth keeping them. Otherwise it introduces unnecessary complexity.
test/decoders/test_mld.py
Outdated
| def repetition_stim(): | ||
| circ = stim.Circuit(""" | ||
| R 0 1 2 | ||
| X_ERROR(0.1) 0 1 2 | ||
| MZZ 0 1 | ||
| DETECTOR rec[-1] | ||
| MZZ 1 2 | ||
| DETECTOR rec[-1] | ||
| M 0 1 2 | ||
| OBSERVABLE_INCLUDE(0) rec[-1] rec[-2] rec[-3] | ||
| """) | ||
| return circ |
There was a problem hiding this comment.
If this is only going to be used in one test (test_mld_repetition) I think it's best to just let it live in that test. It only makes sense to keep it outside the test if other tests are going to use it.
There was a problem hiding this comment.
In fact, now that I take a closer look you have this defined in conftest.py as well
test/decoders/test_mld.py
Outdated
| def repetition_shots(): | ||
| det_shots = np.array([[1, 0], [1, 1], [0, 1], [0, 0]]) | ||
| obs_shots = np.array([[1], [1], [1], [0]]) | ||
| return det_shots, obs_shots |
There was a problem hiding this comment.
Same as above, restrict to the test being used if not used anywhere else
| def _get_env(cls) -> object: | ||
| if cls._env is None: | ||
| import gurobipy as gp | ||
|
|
||
| cls._env = gp.Env() | ||
| return cls._env |
There was a problem hiding this comment.
If you only use this in decode_error I don't think it makes sense to spin this off as a method.
| except ImportError as e: | ||
| raise ImportError( | ||
| "The scipy package is required for GurobiDecoder. " | ||
| 'You can install it via: pip install "bloqade-decoders[mle]"' |
There was a problem hiding this comment.
I think you can just mention pip install scipy here
| for i in range(len(weights)): | ||
| error_variables.append(m.addVar(vtype=GRB.BINARY, name="e" + str(i))) | ||
| objective += weights[i] * error_variables[i] | ||
| m.setObjective(objective, GRB.MAXIMIZE) | ||
|
|
||
| for i in range(len(detector_vertices)): | ||
| detector_variables.append( | ||
| m.addVar( | ||
| vtype=GRB.INTEGER, | ||
| name="h" + str(i), | ||
| ub=len(detector_vertices[i]), | ||
| lb=0, | ||
| ) | ||
| ) | ||
| constraint: gp.LinExpr = gp.LinExpr(0) |
There was a problem hiding this comment.
I think in both for loops here you can use enumerate as opposed to indexing into the same iterable via length
| @overload | ||
| def decode( | ||
| self, | ||
| detector_bits: npt.NDArray[np.bool_], | ||
| verbose: bool = ..., | ||
| return_weights: Literal[False] = ..., | ||
| ) -> npt.NDArray[np.bool_]: ... | ||
|
|
||
| @overload | ||
| def decode( | ||
| self, | ||
| detector_bits: npt.NDArray[np.bool_], | ||
| verbose: bool = ..., | ||
| *, | ||
| return_weights: Literal[True], | ||
| ) -> tuple[npt.NDArray[np.bool_], np.ndarray]: ... |
There was a problem hiding this comment.
I don't think it's worth having the overloads here because it makes things harder to read. I do understand it has benefits for the type checking though but this doesn't seem worth it to me.
|
I'm adding @rafaelha to the review here in case he might have some additional suggestions/alternative feedback to what I've given, especially considering he has more expertise with this stuff (: Overall I think things are alright but I just want to make sure we have tests that make sense and things are structured properly for external users |
- Convert mld.py and mle.py into subpackages with separate modules for decoder logic, sinter interface, and utilities - Move sinter classes to dedicated sinter_interface.py submodules so importing the core decoder does not require sinter - Move helper functions (pack/unpack, shots_to_counts) to mld/utils.py - Remove STEP_SIZE global constant, make it a constructor argument - Remove test-only properties (det_obs_counts, is_cached_correction, is_cached_df, max_observable_index, detector_vertices, weights, observable_indices) - Inline _get_env into decode_error - Fix scipy import error message to suggest pip install scipy - Use enumerate instead of range(len(...)) in decode_error loops - Remove @overload decorators from GurobiDecoder.decode - Remove trivial tests that only test stim DEM or default values - Inline single-use test helpers into their respective tests - Remove SinterTableDecoder/SinterGurobiDecoder from top-level exports Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Errors with probability 1.0 caused a ZeroDivisionError in the log-likelihood weight calculation (log(p/(1-p))). These "certain" errors always fire, so pre-apply their detector and observable contributions instead of including them in the MIP. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
johnzl-777
left a comment
There was a problem hiding this comment.
Looking good! I have one additional small request but I'm quite happy with the changes. Thank you for the additions (:
There was a problem hiding this comment.
Can you actually update/add an import test here? Just to enforce that the import structure that currently exists works as-is? I know in the previous review I encouraged this idea of mle.decoder vs. mle.siner_interface and I think it would be nice to check here.
Summary
GurobiDecoder(MLE) andTableDecoder(MLD) migrated from brute-force-decodersSinterGurobiDecoderandSinterTableDecoderadapters for Sinter integrationmle,mld,sinter) in pyproject.tomlTest plan
🤖 Generated with Claude Code