Skip to content

fix(lda): address review follow-ups from #456 - #457

Merged
Mec-iS merged 2 commits into
mainfrom
fix/lda-review-followups
Aug 25, 2026
Merged

fix(lda): address review follow-ups from #456#457
Mec-iS merged 2 commits into
mainfrom
fix/lda-review-followups

Conversation

@Mec-iS

@Mec-iS Mec-iS commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator

Addresses all review comments from #456:

Bug fixes:

  • Singularity threshold: replaced T::epsilon() (~2.2e-16) with a relative tolerance (1e-4 * l_max), matching sklearn's approach. Near-singular Sw matrices no longer pass the guard and produce garbage whitening.
  • PartialEq: replaced T::epsilon() with a fixed 1e-10 tolerance for float comparison.

Refactoring:

  • Renamed private field scalingsprojection_matrix to avoid shadowing the public scalings() getter (consistent with PCA's components naming).

Test coverage (33 previously uncovered lines):

  • mismatched_x_y_is_rejected — X/y dimension mismatch
  • zero_n_components_is_rejected — n_components < 1 validation
  • transform_wrong_features_is_rejected — wrong feature count on transform

Cleanup:

  • Removed polyfill.io script tag (supply-chain concerns, inconsistent with PCA module)
  • Added blank line between pub mod lda and pub mod pca in mod.rs
  • Bumped version to 0.6.14 and moved CHANGELOG entry from [0.6.13] to [0.6.14]

- Singularity threshold: T::epsilon() → relative tolerance (1e-4 * l_max), matching sklearn
- PartialEq: T::epsilon() → fixed 1e-10 tolerance for float comparison
- Rename field scalings → projection_matrix to avoid shadowing the public getter
- Add missing test coverage: X/y mismatch, zero n_components, wrong transform features
- Remove polyfill.io script tag (supply-chain concerns)
- Add blank line between lda and pca module declarations in mod.rs
- Bump version to 0.6.14 and move CHANGELOG entry accordingly
@Mec-iS

Mec-iS commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator Author

Great follow-up PR — the review comments from #456 are addressed cleanly and consistently. Here are my observations, a mix of approvals and a few minor suggestions.


✅ What looks good

Singularity threshold (tol = max(1e-10, 1e-4 * l_max))
This is the right approach. Using T::epsilon() (~2.2e-16 for f64) was far too tight for a relative check on eigenvalues of Sw. The max(abs_floor, rel_factor * l_max) pattern — matching sklearn's tol parameter logic — correctly guards against near-zero eigenvalues without rejecting well-conditioned problems. Good call documenting the sklearn correspondence in the commit message.

PartialEq tolerance fixed at 1e-10
Correct. T::epsilon() (machine epsilon) is the wrong semantic here: it's a relative unit, not an absolute threshold for comparing trained model parameters. A fixed 1e-10 is appropriate for comparing projection matrices computed in double precision. No objection.

Field rename scalingsprojection_matrix
Clean. The public getter scalings() is preserved for API compatibility, and the internal field no longer shadows it. Consistent with PCA's components naming convention.

polyfill.io removal
Correct and important. polyfill.io has been a documented supply-chain risk since mid-2024. Removing it and keeping only the MathJax CDN link is the right call.

New tests
mismatched_x_y_is_rejected, zero_n_components_is_rejected, and transform_wrong_features_is_rejected are exactly the right boundary tests for a transformer. They're minimal, clearly named, and reuse the existing three_class_data() helper. No issues.


🔍 Minor observations / suggestions

1. PartialEq tolerance 1e-10 is unsafe for f32

let tol = T::from(1e-10).unwrap();

This is fine for f64, but for f32 — which T could be — 1e-10_f32 rounds to 0.0 (f32 min positive normal is ~1.2e-7). That makes (a - b).abs() > 0.0 almost always false, causing nearly all pairs to be considered equal. Consider raising the floor:

let tol = T::from(1e-6).unwrap();  // safe floor for both f32 and f64

Alternatively, document that PartialEq is only meaningful for f64 contexts. This won't break current tests (likely only f64 is exercised), but worth noting.

2. l_max fold style — cosmetic

let l_max = l.iter().cloned().fold(T::zero(), |a, b| if b > a { b } else { a });

Functionally correct. One note: if all eigenvalues of Sw are zero (degenerate all-same-class input), l_max = 0 and tol falls back to 1e-10, which still correctly catches the singular case. ✅ No change needed.

3. CHANGELOG version bump ordering

The PR moves the LDA entry from [0.6.13][0.6.14] and pushes the XGBoost fix to [0.6.13]. This is correct sequencing assuming 0.6.13 was never published. If 0.6.13 was already tagged/released (from #456's bump), inserting a new entry under a published tag could confuse users diffing changelogs. Worth confirming before merging — if it was never published as a crate release, no issue.


Summary

The core fixes (singularity guard, PartialEq tolerance, rename, test coverage, polyfill removal) are all correct and well-executed. The f32 edge case in PartialEq is the only thing worth a follow-up issue if f32 support is intended. Everything else is clean — good work.

1e-10 rounds to 0.0 in f32 (min positive normal ~1.2e-7), making
(a - b).abs() > 0.0 almost always true for distinct values. The 1e-6
floor works correctly for both f32 and f64.

Addresses review feedback on #457.
@Mec-iS
Mec-iS merged commit ea178d2 into main Aug 25, 2026
13 checks passed
@Mec-iS
Mec-iS deleted the fix/lda-review-followups branch August 25, 2026 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant