Fix bugs identified in code review#63
Merged
Merged
Conversation
- NDArrayUtils.h: max() was computing the minimum (copy-paste from min());
corrected to use std::numeric_limits<T>::lowest() and std::max
- module.cpp: fromNDArray() returned a non-owning numpy view into a
soon-to-be-destroyed NDArray; now allocates an owning array and copies
- rcpp_api.cpp: removed duplicate indices/marginals size checks in ipf(),
qis() and qisi() (first check already throws)
- Log.h: removed static from the local char buffer in to_string_impl<T*>
to avoid shared mutable state across calls
- humanleague.R: fixed two malformed Roxygen \itemize entries where a
stray '-' outside \item{} would render incorrectly
- pyproject.toml: moved pandas-stubs to dev dependencies (it is a
typing-only package and not needed by end users at runtime)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
numpy 2.4.1 -> 2.4.5 pandas 2.3.3 -> 3.0.3 pybind11 3.0.0 -> 3.0.4 pytest 8.1.4 -> 9.0.3 ruff 0.12.9 -> 0.15.13 build 1.2.2.post1 -> 1.5.0 ty 0.0.31 -> 0.0.37 pre-commit 4.5.1 -> 4.6.0 pandas-stubs 2.2.3.250308 -> 3.0.0.260204 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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.
Summary
NDArrayUtils.h:max()was computing the minimum due to a copy-paste error frommin()— usedstd::minandstd::numeric_limits<T>::max()(sentinel for a minimum search). Fixed to usestd::numeric_limits<T>::lowest()andstd::max.module.cpp:fromNDArray()returned a non-owning numpy view into an NDArray whose lifetime ends at the return site, causing a potential use-after-free. Now allocates an owning array and explicitly copies the data.rcpp_api.cpp: Removed duplicateindices.size() != marginals.size()checks inipf(),qis(), andqisi()— each had the same check twice, with the second being dead code.Log.h: Removedstaticfrom the localchar buf[]into_string_impl<T*>to eliminate shared mutable state across calls (not thread-safe).humanleague.R: Fixed two malformed Roxygen\itemizeentries where a stray-outside\item{}would render incorrectly in the package documentation.pyproject.toml: Movedpandas-stubsfrom runtimedependenciesto[dependency-groups].dev— it is a typing-only package not needed by end users.Test plan
uv run pytest— Python test suite should pass unchanged (no logic changes to algorithms)R CMD check— Roxygen fix should produce clean docs; no R logic changedfromNDArrayfix: existing Python tests exercise all return paths (qis,qisi,ipf) and would catch memory corruption🤖 Generated with Claude Code