Repository navigation
Conversation
|
@priya-sundaram-dev, please review. It seems like we are losing a lot of tests in these changes. Is there solid value added in the proposed changes? |
|
Thanks @cclauss — I share the concern; I'd hold this one. The value added is real, but it doesn't offset the verifiable test coverage we'd lose. What's lost: the PR replaces three small, heavily-doctested functions with one large OOP class. Concretely it drops 5 of the 7 numeric For a teaching repo, the reproducible, asserted numeric behavior is the most valuable part of the file. Random-init OOP makes deterministic doctests harder, which is likely why they were dropped — but that's exactly the regression we don't want. Scope: OVR + softmax multi-class also pushes well beyond the original single-example scope. That's not automatically bad, but it raises the bar for how well-tested it needs to be. Suggestion for the author: either (a) keep the original functions and their doctests and add softmax as a separate, deterministically-seeded addition with its own numeric doctests; or (b) if going full OOP, seed the RNG and add doctests asserting on the learned probabilities/loss, and restore the sigmoid range tests plus a log-loss doctest. As submitted I'd not merge — the tradeoff reduces tested behavior on an educational file. Happy to re-review once coverage is restored. |
Describe your change:
Checklist:
Summary of Enhancements
Refactored the core layout of
machine_learning/logistic_regression.py. The original implementation was limited to Batch Gradient Descent and binary classification. This upgrade introduces a robust, object-orientedLogisticRegressionclass capable of handling both binary and advanced multi-class datasets entirely from scratch using NumPy.< 1e-6).np.random.seed,np.random.randn,np.random.permutation) to utilize modernnp.random.default_rng()constraints as recommended by NumPy documentation.black,ruff check, andmypy).Verification Results
All required verification suites were executed locally and passed with zero errors: