Add getter for parameters in all model structs.#379
Conversation
Mec-iS
left a comment
There was a problem hiding this comment.
Review: Add getter for parameters in all model structs
Thank you for this PR — the motivation is solid and the coverage across 25 files is impressive. However, there are several recurring implementation issues that should be addressed before merging.
🔴 Critical
assert! + unwrap() is an anti-pattern
Every getter in this PR follows this pattern:
pub fn parameters(&self) -> &KMeansParameters {
assert!(self.parameters.is_some());
&self.parameters.as_ref().unwrap()
}This is redundant: assert! panics in debug builds, then unwrap() would panic independently anyway. The idiomatic Rust approach is:
pub fn parameters(&self) -> &KMeansParameters {
self.parameters.as_ref().expect("parameters not set — model not fitted")
}This applies to all 25 files and should be fixed globally.
Unnecessary Option<T> wrapping
Parameters are stored as Option<T> but are always Some(...) after fit() — there is no legitimate None state post-construction. Consider either:
- Storing parameters as
Tdirectly (not wrapped inOption), or - Returning
Option<&T>from the getter and letting callers handle theNonecase without panicking.
A library API should not expose infallible-looking getters that can silently panic.
🟡 Moderate
No tests added
The PR adds 294 lines with zero new tests. Every getter should have at least a basic round-trip test: fit a model, call .parameters(), assert the returned values match what was passed in. Existing test modules in each file are the right place.
Breaking API changes not flagged
- In
decision_tree_classifier.rs,parameters()was a private trait method and is nowpub fn— this widens the public API and is a semver-breaking change. - In
svc.rs,seed: Option<u64>is changed from private topub seed— this appears to be an unintentional side-effect and is also a public API change.
Both should be explicitly documented in the PR description and changelog, and the maintainers should assess the version bump implications.
🟢 Minor
- Trailing commas: Some struct field additions have trailing commas, others do not (e.g.
ExtraTreesRegressor,DecisionTreeRegressor). Please runcargo fmtbefore merging. Option::None/Option::Somestyle: Several places useOption::NoneandOption::Some(...)instead of the idiomaticNone/Some(...). Not incorrect, but inconsistent with the rest of the codebase.- Doc comments: All getters share the same generic boilerplate. Comments should mention the specific return type and note that the method will panic if called on an unfitted model instance.
- Hidden
.clone()cost: Severalfit()methods now clone fields before storingparameters(e.g.parameters.alpha.clone(),parameters.distance.clone()). This is necessary given the design, but worth acknowledging — especially for types like distance functions that may be non-trivially cloneable.
Summary of requested changes
- Replace all
assert! + unwrap()with.expect("..."). - Re-evaluate
Option<T>wrapping — store asTdirectly or returnOption<&T>. - Add at least one test per getter verifying round-trip correctness.
- Explicitly document and flag the breaking API changes (
DecisionTreeClassifier::parametersvisibility,SVCParameters::seedvisibility). - Run
cargo fmtto resolve trailing comma and style inconsistencies.
⛔ Build broken: serialisation (
|
|
Thanks @HubertMajewski |
Fixes #378
Current behaviour
Several fitted models do not expose the hyperparameters that were used to create or train them.
After a model is fitted, and especially after it is serialized and deserialized, users can inspect learned model state such as coefficients, intercepts, trees, classes, or other fitted attributes where available, but the original training parameters are not consistently accessible from the model instance.
This makes it difficult to:
New expected behaviour
Fitted models expose a public
.parameters()getter that returns the hyperparameters used to create or train the model.The getter provides read-only access to the model’s parameter struct, allowing users to inspect model configuration after fitting and after deserialization where the parameters are stored with the model.
This improves model reproducibility, auditability, debugging, and experiment tracking without requiring users to separately persist parameter structs outside the model.
Change logs
Added
.parameters()getter methods to fitted model types that store their training hyperparameters.