Add missing llr2bitProb function to optic.utils - #34
Conversation
`optic.comm.modulation` imports `llr2bitProb` from `optic.utils`, but the
function is not defined anywhere in the package. As a result, importing
`optic.comm.modulation` (and therefore most of the library) fails on the
current main branch with:
ImportError: cannot import name 'llr2bitProb' from 'optic.utils'
The helper is used by `softEstimator` (optic/comm/modulation.py) and by
`calcExtrLLR` (optic/comm/metrics.py), both of which call it as
`llr2bitProb(-llr)`. Since the LLRs produced by `calcLLR` follow the
log(P(b=0)/P(b=1)) convention, the expected mapping is the logistic
function, so that P(b=1) = 1/(1 + exp(LLR)).
This adds the function to `optic.utils` as a Numba-compiled routine, so it
can be called from the jitted functions above, and adds the missing import
in `optic.comm.metrics`, where `llr2bitProb` was used without being
imported.
Reviewer's GuideThis PR restores importability and soft-metric functionality by adding the missing llr2bitProb helper to optic.utils (with Numba support) and wiring its usage/import into the metrics module. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The docstring for llr2bitProb defines LLRs as log(P(b=1)/P(b=0)) while noting the rest of the library uses log(P(b=0)/P(b=1)); consider aligning conventions (or renaming the function) to avoid confusion and reduce the need for sign flips at call sites.
- llr2bitProb uses the naive logistic formulation 1/(1+exp(-llr)); for very large |llr| values this can cause numerical under/overflow, so consider adding clipping or a more numerically stable implementation to handle extreme LLRs.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The docstring for llr2bitProb defines LLRs as log(P(b=1)/P(b=0)) while noting the rest of the library uses log(P(b=0)/P(b=1)); consider aligning conventions (or renaming the function) to avoid confusion and reduce the need for sign flips at call sites.
- llr2bitProb uses the naive logistic formulation 1/(1+exp(-llr)); for very large |llr| values this can cause numerical under/overflow, so consider adding clipping or a more numerically stable implementation to handle extreme LLRs.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Hello @Hewei603! Thanks for your warning! In fact, this issue was a result of a few changes and additions I have been doing in the repository this last week. The changes with I have updated the repository now, so please check if all is fine. So, thanks, but the pull request won't be needed this time. However, if you want to update/check the tests, please feel free to do it. It would be really valuable. Unfortunatelly, I don't have time to write a better and more complete set of tests for the repository, although the examples folder kind of indireclty assumed this task So, any help with this would be very welcome. |
|
Thanks for the quick turnaround, and for explaining the context — I confirmed that I would be glad to take up the tests. I opened #35 with a first pass: the failing A few things surfaced while writing the tests — a one-sample offset at the output of |
2 participants
Problem
The current
mainbranch cannot be imported.optic/comm/modulation.pyimportsllr2bitProbfromoptic.utils, but that function is not defined anywhere in the package:Since
optic.comm.modulationis imported byoptic.comm.metrics,optic.models.tx,optic.dsp.equalizationandoptic.dsp.carrierRecovery, this breaks essentially the whole library, including all the example notebooks.The import was introduced in cee1ca6, but the function itself was never committed.
What was expected
llr2bitProbis referenced in two places, both asllr2bitProb(-llr):softEstimator()inoptic/comm/modulation.pycalcExtrLLR()inoptic/comm/metrics.pycalcLLR()produces LLRs following thelog(P(b=0)/P(b=1))convention, soPb1 = llr2bitProb(-llr)is consistent with the logistic functionChanges
llr2bitProb()tooptic/utils.py, decorated with@njit(cache=True)so it can be called from the jittedsoftEstimator()andcalcExtrLLR()routines, and list it in the moduleautosummary.llr2bitProbimport inoptic/comm/metrics.py, where the function was used but never imported (which would raise aNameErrorincalcExtrLLR()even after the import error above is fixed).Verification
Tested on Python 3.14 with numpy 2.4.6 / scipy 1.18.0 / numba 0.66.0:
pytest tests/runs (test_dsp.pyandtest_modulation.pypass).manakovSSF+pdmCoherentReceiver+edc+mimoAdaptEqualizer+cpr) gives BER = 0, SNR ≈ 22.7 dB, GMI = 4.00 bits/symbol, as expected.Note (unrelated to this PR)
tests/test_metrics.py::test_fastBERcalcstill fails, but for an independent reason: it callsawgn(symbTx, snrdB)passing a float where aparametersobject is expected. Becauseawgn()reads its configuration withgetattr(param, "snr", 20), the SNR silently falls back to the 20 dB default and the measured BER no longer follows the sweptEbN0values. I can open a separate PR/issue for that if you'd like.Summary by Sourcery
Restore importability and soft-metric computations by adding the missing LLR-to-bit-probability utility and wiring it into metrics.
New Features:
Bug Fixes:
Enhancements: