Rework the test suite and add a CI workflow - #35
Merged
edsonportosilva merged 2 commits intoAug 1, 2026
Conversation
The existing tests mixed unittest and pytest styles and covered only a few functions of optic.dsp.core, optic.comm.modulation and optic.comm.metrics. test_metrics.py::test_fastBERcalc was also failing: it called `awgn(symbTx, snrdB)` passing a float, but awgn() reads its configuration through `getattr(param, "snr", 20)`, so the SNR silently fell back to the 20 dB default and the measured BER no longer followed the EbN0 values being swept. The test now builds a proper parameters object, which makes the measured curve match theoryBER() to within 20%. Changes: - Port every test to pytest, grouped in classes by the function under test. - tests/test_dsp.py: add coverage for pnorm, signalPower, sigPow, firFilter, pulseShape, upsample, decimate, resample, quantizer, lowPassFIR, freqShift, movingAverage, delaySignal, gaussianNoise, phaseNoise and symbolSync. - tests/test_modulation.py: add coverage for the constellation generators, the detector and the Gray mapping property that nearest-neighbour symbols differ in a single bit. - tests/test_metrics.py: fix the AWGN setup and add coverage for theoryBER, Qfunc, calcEVM, calcLLR and the GMI/MI estimators. - tests/test_channels.py: new, covering awgn, linearFiberChannel, its inversion by edc, and the reduction of ssfm to the linear channel when the nonlinear coefficient is zero. - tests/test_utils.py: new, covering the unit conversions, the bit array helpers, dotNumba and llr2bitProb. - tests/test_imports.py: new, checking that every module can be imported. GPU modules are skipped when CuPy is unavailable. - Ignore __pycache__ and .pytest_cache directories. The suite runs in about 12 s and passes with 182 tests (3 skipped without a GPU).
Runs pytest on Python 3.10 to 3.13 for pushes to main and for pull requests. Together with tests/test_imports.py this catches breakages such as an import of a name that does not exist, which otherwise leave the whole package unusable until someone runs the code by hand.
edsonportosilva
self-requested a review
August 1, 2026 02:33
edsonportosilva
approved these changes
Aug 1, 2026
Owner
|
Hello, @Hewei603! I have reviewed your changes and I believe they make sense for all the tests that were improved and the new added ones. Thanks for contributing with the repository! Please, open an issue listing these last findings you mentioned. I can have a look on them later. Perhaps, a few of them are already being looked at in some upcoming pull requests. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Following up on your suggestion in #34 — this reworks the test suite and adds a CI workflow.
Why
tests/test_metrics.py::test_fastBERcalcwas failing onmain. It callspassing a float, while
awgn()reads its configuration throughgetattr(param, "snr", 20). The SNR therefore silently fell back to the 20 dB default and the measured BER no longer followed theEbN0values being swept, so it could never matchtheoryBER(). Building a properparametersobject makes the measured curve match the theory to within 20%.The rest of the suite mixed
unittestandpyteststyles and covered only a handful of functions, so I extended it while I was there.What is in this PR
tests/test_utils.pydotNumba,llr2bitProb(including its numerical stability for very large LLRs)tests/test_dsp.pypnorm,signalPower,sigPow,firFilter,pulseShape,upsample,decimate,resample,quantizer,lowPassFIR,freqShift,movingAverage,delaySignal,gaussianNoise,phaseNoise,symbolSynctests/test_modulation.pydetector, and the Gray mapping property that nearest-neighbour symbols differ in a single bittests/test_metrics.pytheoryBER,Qfunc,calcEVM,calcLLRand the GMI/MI estimatorstests/test_channels.pyawgn,linearFiberChannel, its inversion byedc, and the reduction ofssfmto the linear channel forgamma = 0tests/test_imports.py.github/workflows/tests.ymlmainand pull requestsWherever possible the assertions check a property rather than a hardcoded number: RRC pulses are verified through the Nyquist criterion after matched filtering,
linearFiberChannelthrough the fiber loss coefficient and energy conservation,phaseNoisethrough the variance of its increments,awgnthrough the SNR measured at its output. Random number generators are seeded, and the whole suite runs in about 12 s (182 tests, 3 skipped without a GPU).tests/test_imports.pyexists specifically because of #34: an import of a name that does not exist leaves the whole package unusable, and running it in CI makes that visible immediately.Things I noticed while writing the tests
None of these are addressed here, since they are outside the scope of a test PR. Happy to open issues or separate PRs for any of them.
edc()shifts the signal by one sample. Compensating 200 km of dispersion on an RRC-shaped QPSK signal leaves a normalized MSE of 2.1e-1; realigning the output by a single sample brings it down to 5.8e-3. The offset comes fromblockwiseFFTConv()—delaySignal()works around the same effect withnp.roll(delayedSig, -1), butedc()does not. In a full receiver the adaptive equalizer absorbs it, which is probably why it went unnoticed. The test aligns the sequences before comparing, so it passes either way.delaySignal(sig, 0)corrupts the last sample. Withdelay = 0there is no zero padding, so the finalnp.roll(..., -1)shifts a sample out of the signal. Delays greater than zero are unaffected.pnorm()docstring does not match its behaviour. It says the average power of each component is normalized, but the implementation isx / sqrt(mean(x * conj(x))), where the mean runs over the whole array. For a dual-polarization signal the total average power becomes 1 per sample rather than 1 per mode. The behaviour looks intentional — it preserves the power ratio between polarizations — so this may just be a documentation fix.calcEVM()returns the squared EVM. It computesmean(|error|^2) / mean(|reference|^2), whereas EVM is usually reported as the square root of that ratio. The example notebooks print the returned value as a percentage, so the reported figure is currently the error power ratio, not the RMS EVM.Two deprecated imports that will eventually break.
optic/plot.pyimportsgaussian_filterfromscipy.ndimage.filters, a namespace scheduled for removal in SciPy 2.0, andoptic/models/amplification.pyimportsnumpy.matlib, deprecated since NumPy 1.19. Both currently emit warnings.quantizer()only accepts 2D arrays, since it indexesx.shape[1]. Every internal caller reshapes first, so this is just worth a note in the docstring.