Repository navigation
Conversation
78a68d4 to
80d6e12
Compare
|
Thanks @jolars, this is a well-tested PR and the memory reduction is real. I reviewed the diff (head What looks good
Blocking / should address
Suggestions
ScopeOn your question about sparse support: I'd recommend keeping this PR dense-only and landing sparse in a follow-up after #455 is settled. On the example: keep it. Happy to re-review once the feature-gating design (1) and the Elastic Net slowdown (2) are addressed. @Mec-iS, you were requested as a reviewer, so tagging you for the dependency and design decisions. |
|
check |
|
Thanks for the review! I have fixed the feature-unificiation problem. I'm looking into the elastic net performance regression. But some performance regression is to be expected, given that this is essentially a trade-off between memory and speed (in the dense matrix case). |
|
@Mec-iS - do you want me to break the |
Thanks, but can you please clarify what you mean? That I should use criterion here as well for the benchmarks? These were intended mostly to give you an idea of the performance tradeoff, but I can add Criterion as a depenency here if you'd like? That repo doesn't have any lasso or elastic net benchmarks, but if you'd like me to add those to that repo I'd be happy to do it too. But I'm not sure if it would make a difference for this PR, or? |
|
yes, for me to accept these changes I need complete information about how/what they improve in quantified terms. So we need to run comparative benchmarks in |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #480 +/- ##
===========================================
+ Coverage 43.97% 63.47% +19.49%
===========================================
Files 85 97 +12
Lines 7281 8797 +1516
===========================================
+ Hits 3202 5584 +2382
+ Misses 4079 3213 -866 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Got it! I'll remove the example and run a benchmark through your benchmark suite instead. I'll also add benchmarks for elastic net/lasso there in a separate PR. |
|
I've improved code coverage a bit too, but the missing coverage is mainly due to the currrent call to taupaulin missing some parts. I'm filing a separate PR to update that workflow. |
|
I've now run the benchmarks in smartcorelib/smartcore-benches#9. I compared #483 with both the eager and lazy paths in this PR. Using #483 as the baseline gives both versions the ndarray iterator fix. Lazy was faster in all 24 cases, by 14-51% for Lasso and 13-57% for Elastic Net. I didn't see a slowdown above 2% with the eager path, which is still the default. Here are the results for 65,536 rows and 32 columns with a row-major
That's about 42% less peak memory for Lasso and 59% less for Elastic Net. These memory numbers cover the whole process, including input creation. I wasn't expecting it to be faster, so I checked Gram construction separately. For the same size and layout, it takes 197 ms with eager Lasso versus 21 ms with lazy, and 155 ms versus 21 ms for Elastic Net. Across all cases, lazy Gram construction was 86-89% faster. The resulting Gram matrices were exactly equal. This seems to explain the speedup. The lazy kernel computes one triangle and mirrors it, and it works on small panels. The eager kernel computes both triangles separately. Elastic Net also avoids storing and traversing the identity block. I haven't measured those changes individually. The Gram savings are larger than the total fit savings in the large row-major case, so lazy appears to spend more time elsewhere. Improving the eager Gram kernel could recover much of this benefit too. I also compared the fitted models on every dataset. The baseline and PR eager path gave exactly equal coefficients, intercepts, training MSE, and objective values. Between eager and lazy, the largest coefficient difference was 1.2e-8, the largest prediction difference was 3.9e-8, and the largest relative objective difference was 2.9e-10. For reference, I ran this on an AMD Ryzen 9 7900 with NixOS 26.11 and Rust 1.99.0, using Cargo release defaults and pinning the runs to logical CPU 2. Criterion used 30 samples, a one-second warmup, and three seconds of measurement time, extended where needed. Data creation was outside the timed loop, but model destruction was included. The memory figures are medians from five fresh processes per version and case, alternating between versions. The benchmarks cover 1,024, 8,192, and 65,536 rows with 32 columns, plus 1,024 rows with 128 columns. Each uses row-major and column-major The commits were This is one full timing run per version on one machine, with fixed synthetic data. I'd expect the trade-off to depend on the data and hardware, but these results look promising. The full timing table is below. Full fit-time results (means in milliseconds)
Negative changes mean faster fits. |
Closes #479
Current behavior
Lasso copies the design matrix to center and scale its columns. Elastic Net also stores an augmented design matrix with an identity block for the L2 penalty. These allocations increase memory use during fitting.
New expected behavior
The optional
lazy-normalizationfeature lets Lasso and Elastic Net apply normalization to a borrowed design matrix throughlazymatrix. Elastic Net also applies the identity block without storing the augmented matrix. BothDenseMatrixandndarray-bindingsare supported.Enable the feature, then select the lazy path with
LassoParameters::default().with_lazy_normalization(true)orElasticNetParameters::default().with_lazy_normalization(true). The eager path remains the default, including when another dependency enables the feature. The existingnormalizeoption still controls whether columns are centered and scaled. Parameter searches use a fixedlazy_normalizationsetting.The lazy path reduces memory use but adds repeated arithmetic. Fit time can increase for both estimators, including Elastic Net on large inputs. The solver still stores a dense Gram matrix with one row and column per feature. Comparative time and memory benchmarks in
smartcore-benchesremain pending. The local benchmark example and results CSV have been removed.This PR covers dense matrices only. Sparse matrix support remains separate from this change, pending #455.
Breaking change and release target
This PR targets 0.7 because it includes a breaking source compatibility change. Enabling
lazy-normalizationadds a publiclazy_normalizationfield toLassoParameters,LassoSearchParameters,ElasticNetParameters, andElasticNetSearchParameters. Downstream struct literals without..Default::default()must addlazy_normalization: falseto retain the eager path, ortrueto select the lazy path. This also applies when another dependency enables the feature through Cargo feature unification. Missing fields default tofalseduring deserialization.lazymatrix0.6.0 declares Rust 1.85 support and anMIT OR Apache-2.0license. It is an optional dependency with default features disabled.The ndarray
from_iteratorfix is in #483. This branch still includes it because the ndarray tests need it. Once #483 merges, this branch must be updated frommainto remove the fix from this PR's diff.Change logs
ndarray-bindings. Existing all-feature jobs also cover MSRV and WebAssembly.Validation
The following commands passed locally on NixOS with Rust 1.99.0:
cargo fmt --all -- --check cargo build --all-features cargo test --all-features cargo clippy --all-features -- -Drust-2018-idioms -Drust-2024-compatibility -DwarningsThe test run passed 612 unit tests, 34 integration tests, and 75 doctests. It reported five unfulfilled lint expectations in test builds outside the changed files. MSRV and WebAssembly checks were not run locally for this update.