Add getter for parameters in all model structs.

[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:

• 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.
• Added parameter round-trip tests that verify the values returned by .parameters() match the values supplied during fitting.

Changed

• Exposed DecisionTreeClassifier::parameters() as a public method.
• Exposed SVCParameters::seed as a public field.
• Created custom Deserialize implementation to support deserialization without requiring Default to the public API.

[Mec-iS]

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]

⛔ 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]

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

[HubertMajewski]

Hi @Mec-iS

I initially committed them to unblock on a project I am working on. These changes are highly reasonable; I would be more than happy to apply these fixes.

[Mec-iS]

Please run cargo build --all-features to check the build locally.

[HubertMajewski]

@Mec-iS Please feel free to review now and let me know of any other additional changes.

[Mec-iS]

The lint check is still failing. Please follow the commands as in CONTRIBUTING.md instructions, in particular run cargo clippy .... you can read where the linting fails at https://github.com/smartcorelib/smartcore/actions/runs/29898188107/job/88903474713?pr=379

This is quite a massive PR with 1500+ lines of code so we need to be careful to add real value for the user relative to the complexity we add in the code.

Thanks again for this PR @HubertMajewski — exposing .parameters() consistently is a genuinely useful addition. After reviewing the source code in depth, here are the issues found, grouped by severity.

---

🟠 High Severity

1. SVC::parameters() silently returns None after deserialization

SVC stores its parameters as a lifetime-bound borrow:

parameters: Option<&'a SVCParameters<TX, TY, X, Y>>,

Because a reference cannot be serialized, the field is annotated with #[serde(skip)], meaning any deserialized SVC will always return None from .parameters(). The test test_returns_none_on_no_parameters explicitly validates this degraded state rather than a successful round-trip — which is the opposite of what the PR description promises ("Added support for retrieving model hyperparameters after deserialization where applicable").

Compare this with LogisticRegression, which stores parameters by value (owned Option<LogisticRegressionParameters<TX>>), enabling a true serde round-trip that is correctly tested.

Recommendation: Either clone SVCParameters into the fitted SVC at fit time (taking ownership, matching the LogisticRegression pattern), or explicitly document in the SVC::parameters() doc comment that this method always returns None after deserialization, so callers are not silently misled.

---

2. MultiClassSVC has no .parameters() getter

SVC (binary) received a .parameters() getter, but MultiClassSVC — which wraps multiple SVC instances and accepts the same SVCParameters — did not. This leaves the API inconsistent: a user working with multi-class SVM cannot inspect the parameters used, defeating one of the stated goals of the PR.

Recommendation: Add a parameters() getter to MultiClassSVC, or add a code comment explicitly justifying the omission.

---

🟡 Medium Severity

3. Custom Deserialize impl — correct, but needs a comment

The hand-written Deserialize for LogisticRegressionParameters uses a private helper struct to avoid requiring a Default bound on the generic type T. The implementation is sound and the pattern is a recognised Serde idiom. However, there is no doc comment explaining why #[derive(Deserialize)] is insufficient here. Future maintainers may not understand why the custom impl exists and could accidentally replace it with a derive.

Recommendation: Add a short // SERDE: manual impl avoids requiring Default on T; #[derive(Deserialize)] would add an implicit Default bound comment above the impl block. Also verify that all other parameter structs changed in this PR apply the same strategy consistently (or use #[serde(default)] with an explicit Default impl where appropriate).

---

4. Audit for missing models

The src/ tree contains: cluster, decomposition, ensemble, linear (5 models), naive_bayes, neighbors, svm (svc, svr), tree, and xgboost. The PR touches 25 files, but it is not immediately clear from the PR description whether svr.rs, the naive_bayes variants, neighbors (KNN), decomposition (PCA/SVD), and xgboost trees all received .parameters() getters. Any omissions will leave the API inconsistent.

Recommendation: Add a checklist to the PR description explicitly listing every model struct that was updated and any that were intentionally skipped (with justification).

[codecov]

Codecov Report

❌ Patch coverage is 17.17791% with 135 lines in your changes missing coverage. Please review.
✅ Project coverage is 43.94%. Comparing base (70d8a0f) to head (10a1d59).
⚠️ Report is 27 commits behind head on development.

Files with missing lines	Patch %	Lines
src/neighbors/knn_classifier.rs	12.50%	14 Missing ⚠️
src/neighbors/knn_regressor.rs	12.50%	14 Missing ⚠️
src/cluster/dbscan.rs	0.00%	12 Missing ⚠️
src/tree/base_tree_regressor.rs	33.33%	12 Missing ⚠️
src/naive_bayes/bernoulli.rs	16.66%	10 Missing ⚠️
src/tree/decision_tree_classifier.rs	44.44%	10 Missing ⚠️
src/linear/logistic_regression.rs	0.00%	9 Missing ⚠️
src/naive_bayes/multinomial.rs	33.33%	6 Missing ⚠️
src/naive_bayes/categorical.rs	0.00%	5 Missing ⚠️
src/cluster/agglomerative.rs	0.00%	3 Missing ⚠️
... and 15 more

Additional details and impacted files

@@               Coverage Diff               @@
##           development     #379      +/-   ##
===============================================
- Coverage        45.59%   43.94%   -1.66%     
===============================================
  Files               93       95       +2     
  Lines             8034     8172     +138     
===============================================
- Hits              3663     3591      -72     
- Misses            4371     4581     +210     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.