cppcheck quality pass + V2 numerical framework removal — v3.5.0#7
Merged
cppcheck quality pass + V2 numerical framework removal — v3.5.0#7
Conversation
missingOverride: ~DistributionBase() now marked override. shadowFunction: rename local sum accumulators to row_sum/col_sum in BasicMatrix::row_sums() and column_sums() to avoid shadowing sum(). functionStatic: mark 11 member functions static — validateParameters (LogNormal, NegativeBinomial, Pareto, Weibull), isValidCount (Poisson), ligamma (Gamma), readFromStream (XMLFileReader), writeToStream (XMLFileWriter), accumulate_gamma and accumulate_xi (BaumWelchTrainer), flattenObservationLists (SegmentalKMeansTrainer). Matching const removed from BaumWelchTrainer definitions. constParameterReference: add cppcheck-suppress comments on ErrorRecovery methods (recoverFromOverflow, handleNaNValues, repairDegenerateMatrix), ViterbiTrainer helpers (accum_sequence, normalize_and_commit), and fb_crossover_sweep::time_mode. All flagged parameters are genuinely mutated; cppcheck flags early-return paths as sufficient to be const. Co-Authored-By: Oz <oz-agent@warp.dev>
Each tool's parse_pos/parse_positive_int helper re-throws on bad CLI arguments, but main() had no enclosing catch. A non-integer or non-positive argument would call std::terminate instead of printing a usage message. Fix: wrap the argument-parsing block in each main() with try/catch(const std::exception&) that prints the error and returns 1. hmm_validator.cpp additionally had a bare std::stoi call for the T argument outside any try block; replaced with the same pattern. Co-Authored-By: Oz <oz-agent@warp.dev>
BetaDistribution: remove getProbabilityBatch() and getLogProbabilityBatch() (vector-based, pre-span signatures). getBatchLogProbabilities(span, span) and getCumulativeProbability() already exist as the correct interface. NegativeBinomialDistribution: rename CDF() to getCumulativeProbability() to match the naming convention used by Gamma, Poisson, Pareto, Beta and ChiSquared. Update test to use the new name. Co-Authored-By: Oz <oz-agent@warp.dev>
Extract private init_uniform() helper so the constructor can initialise to uniform probabilities without calling the virtual reset(). Virtual dispatch is inactive during construction, so calling an override there is misleading and correctly flagged by cppcheck. reset() now delegates to init_uniform(), keeping the logic in one place. No behaviour change. Co-Authored-By: Oz <oz-agent@warp.dev>
ConvergenceDetector, AdaptivePrecision, ErrorRecovery and NumericalDiagnostics were scaffolding for scaled (non-log-space) calculators and trainers that were removed in the V3 architecture pivot. Several NumericalSafety helpers (safeLog, safeExp, clampLogProbability, checkProbability, checkLogProbability, checkMatrixFinite, checkVectorFinite, isProbabilityDistribution) were part of the same framework and have had no callers since the pivot. Keep the three methods that the V3 code actively uses: NumericalSafety::checkFinite NumericalSafety::clampProbability NumericalSafety::normalizeProbabilities Prune test_numerical_stability.cpp to cover only those three methods. Co-Authored-By: Oz <oz-agent@warp.dev>
Delete the three files that comprised the V2 numerical safety framework — now fully orphaned after the previous commit removed the other four classes (ConvergenceDetector, AdaptivePrecision, ErrorRecovery, NumericalDiagnostics): include/libhmm/math/numerical_stability.h src/common/numerical_stability.cpp tests/common/test_numerical_stability.cpp Remove src/common/numerical_stability.cpp from LIBHMM_SOURCES in CMakeLists.txt and add_hmm_test(test_numerical_stability) from tests/CMakeLists.txt. Update docs: TESTING_STRATEGY.md directory listing and test count (38 → 37); CHANGELOG.md [Unreleased] section with removal note. 37/37 tests pass. Co-Authored-By: Oz <oz-agent@warp.dev>
cppcheck quality pass and V2 numerical framework removal. Co-Authored-By: Oz <oz-agent@warp.dev>
Co-Authored-By: Oz <oz-agent@warp.dev>
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
1 participant
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.
Applied cppcheck 2.20's first full pass against libhmm (alongside pylint/flake8 on pylibhmm), addressed all actionable findings, and removed the last of the pre-V3 scaled-calculator infrastructure.
Changes
Bug fixes (correctness)
• DiscreteDistribution: constructor called virtual reset() where virtual dispatch is inactive — extracted private init_uniform() helper called from both the constructor and reset() so the logic lives in one place and the vtable issue is gone
• 4 tool executables (bw_hotspot, fb_contour_sweep, hotspot_breakdown, hmm_validator): parse_pos/parse_positive_int re-throws were unhandled in main() — wrapped argument-parsing blocks in try/catch so bad CLI arguments produce a clean error message instead of std::terminate
Code quality
• ~DistributionBase: add missing override specifier
• BasicMatrix::row_sums / column_sums: rename local sum accumulator to avoid shadowing the sum() member function
• 11 member functions that don't access this marked static (validateParameters across 5 distributions, isValidCount, ligamma, readFromStream, writeToStream, accumulate_gamma, accumulate_xi, flattenObservationLists)
• 7 constParameterReference false positives suppressed with // cppcheck-suppress comments (parameters genuinely mutated on non-early-return paths)
API symmetry
• BetaDistribution: removed dead getProbabilityBatch(vector, vector) and getLogProbabilityBatch(vector, vector) — orphaned pre-span signatures; getBatchLogProbabilities(span, span) and getCumulativeProbability already existed as the correct interface
• NegativeBinomialDistribution::CDF() → getCumulativeProbability() to match the naming convention used by every other distribution; test updated
Dead code removal (V2 numerical framework)
• Removed include/libhmm/math/numerical_stability.h, src/common/numerical_stability.cpp, tests/common/test_numerical_stability.cpp — NumericalSafety, ConvergenceDetector, AdaptivePrecision, ErrorRecovery, and NumericalDiagnostics were built for the pre-V3 scaled-calculator architecture and have had no callers since the log-space-only pivot. cppcheck confirmed every method was unreachable.
Docs / version
• CHANGELOG.md: [3.5.0] entry; historical V2 entries untouched
• README.md: version badge and test count updated
• tests/TESTING_STRATEGY.md: directory listing and test count updated (38 → 37)
• CMakeLists.txt: VERSION 3.5.0
37/37 tests pass. No behavioural changes to the live HMM/calculator/trainer/distribution API.