Skip to content

Add getter for parameters in all model structs.#379

Open
HubertMajewski wants to merge 1 commit into
smartcorelib:developmentfrom
HubertMajewski:development
Open

Add getter for parameters in all model structs.#379
HubertMajewski wants to merge 1 commit into
smartcorelib:developmentfrom
HubertMajewski:development

Conversation

@HubertMajewski

Copy link
Copy Markdown

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:

  • inspect a loaded model’s configuration,
  • debug model behavior after deserialization,
  • log model metadata,
  • reproduce training settings,
  • compare two fitted models,
  • and verify that a loaded model was trained with the expected hyperparameters.

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

  • Added public .parameters() getter methods to fitted model types that store their training hyperparameters.
  • Added or updated parameter storage on fitted models where needed so the original training parameters can be inspected after fitting.
  • Added support for retrieving model hyperparameters after deserialization where applicable.

@Mec-iS Mec-iS left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 T directly (not wrapped in Option), or
  • Returning Option<&T> from the getter and letting callers handle the None case 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 now pub fn — this widens the public API and is a semver-breaking change.
  • In svc.rs, seed: Option<u64> is changed from private to pub 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 run cargo fmt before merging.
  • Option::None / Option::Some style: Several places use Option::None and Option::Some(...) instead of the idiomatic None / 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: Several fit() methods now clone fields before storing parameters (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

  1. Replace all assert! + unwrap() with .expect("...").
  2. Re-evaluate Option<T> wrapping — store as T directly or return Option<&T>.
  3. Add at least one test per getter verifying round-trip correctness.
  4. Explicitly document and flag the breaking API changes (DecisionTreeClassifier::parameters visibility, SVCParameters::seed visibility).
  5. Run cargo fmt to resolve trailing comma and style inconsistencies.

@Mec-iS

Mec-iS commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

⛔ Build broken: serialisation (serde) failures

This PR breaks compilation with --all-features due to the new parameters fields added to model structs. The CI build (cargo build --all-features) fails with E0277 errors because the parameter types being stored do not satisfy the Default trait bound required by serde's derived Deserialize implementation.

Confirmed failures on at least the following structs:

  • DBSCAN / DBSCANParameters<TX, D>D: Default not satisfied (src/cluster/dbscan.rs:67)
  • LogisticRegression / LogisticRegressionParameters<TX>TX: Default not satisfied (src/linear/logistic_regression.rs:181)
  • BernoulliNB / BernoulliNBParameters<TX>TX: Default not satisfied (src/naive_bayes/bernoulli.rs:360)
  • KNNClassifier / KNNClassifierParameters<TX, D>D: Default not satisfied (src/neighbors/knn_classifier.rs:86)

The root cause is that the derive macro for Deserialize requires all fields to implement Default (for the missing-field fallback path), but the type parameters on these structs (particularly distance functions D and float types TX) do not implement Default. Because the PR did not run the full test suite before submitting, these regressions went undetected.

How to fix

For each affected struct, the options are:

  1. Add #[serde(skip)] to the parameters field — this is the simplest fix if backward-compatible serialisation is acceptable (deserialized models will have parameters: None):

    #[cfg_attr(feature = "serde", serde(skip))]
    parameters: Option<DBSCANParameters<TX, D>>,

    Note: this means parameters are not round-tripped through serde, which partially undermines the stated goal of the PR.

  2. Implement Deserialize manually for the affected structs — more work, but preserves full serde support.

  3. Add + Default bounds to the type parameters — as suggested by the compiler, but this is a breaking change to the public API and would ripple across many downstream users.

Option 1 is the pragmatic starting point but the trade-off must be documented clearly.


Please run the full test suite including the serde feature flag before pushing updates, as described in the CONTRIBUTING guide:

cargo test --all-features
cargo build --all-features

This PR should not be merged until these compilation errors are resolved.

@Mec-iS

Mec-iS commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

Thanks @HubertMajewski
Let's me know if you want to proceed with the fixes yourself

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.

Add getter methods for model hyperparameters after deserialization

2 participants