implemented xgdboost_regression#314
Conversation
| /// * `lambda` - L2 regularization term on weights. | ||
| /// * `gamma` - Minimum loss reduction required to make a further partition. | ||
| pub fn fit( | ||
| data: &Vec<Vec<f64>>, |
There was a problem hiding this comment.
why are we using allocation here?
all the fit(...) methods should have a similar signature and use the Array types. Like you did in:
impl<TX: Number, TY: Number, X: Array2<TX>, Y: Array1<TY>> AgglomerativeClustering<TX, TY, X, Y> {
/// Fits the agglomerative clustering model to the data.
///
/// # Arguments
/// * `data` - A reference to the input data matrix.
/// * `parameters` - The parameters for the clustering algorithm, including `n_clusters`.
///
/// # Returns
/// A `Result` containing the fitted model with cluster labels, or an error if
pub fn fit(data: &X, parameters: AgglomerativeClusteringParameters) -> Result<Self, Failed> {
let (num_samples, _) = data.shape();
let n_clusters = parameters.n_clusters;
if n_clusters > num_samples {
return Err(Failed::because(
FailedError::ParametersError,
&format!(
"n_clusters: {n_clusters} cannot be greater than n_samples: {num_samples}"
),
));
}Same in other parts, whenever we use Vec we should ask if it is not the case to use Array. Vec can be used for labels and sequence that we are sure are relatively small order of magnitude (usually 100-300 elements). For everything that is 2D or require large arrays, the Array interface should be implemented using DenseMatrix or the Array traits.
|
|
||
| /// Predicts the output values for a dataset. | ||
| pub fn predict(&self, data: &[Vec<f64>]) -> Vec<f64> { | ||
| data.iter().map(|row| self.predict_for_row(row)).collect() |
There was a problem hiding this comment.
as above this iteration should use the .iterator(0).copied() implementation. There is a custom iterator for Array, but if you don't use Array you cannot access it. This is also important for compatibility with other libraries like ndarray
|
Thanks 👍🏼 |
|
resolved the problems |
|
I think the lints are resolved |
2 participants
No description provided.