Dataset improvements

[Sauro98]

Hi, I'm opening the PR a little early because I'd like to ask some questions. This PR wants to make the changes proposed in #70.

• struct Dataset has been renamed DatasetBase
• All crates have been updated according to the renaming of Dataset into DatasetBase
• New types have been added:
  • Dataset: owned Array2 as records, owned Array1 as targets. The current implementation allows to write Dataset<f24,f24> or Dataset<f24,usize> and so on. Is this in line with the objective of the proposed changes?
  • DatasetView: view of an Array2 as records, view of an Array1 as targets. The current implementation allows to write DatasetView<f24,f24> or DatasetView<f24,usize> and so on. Calling view() on a Dataset produces a DatasetView.
• Functions inside sub-crate datasets have been changed to return an instance of Dataset.

There's still k-folding missing but I figured that it would be better to have an intermediate check. I also added tests to ensure the existence of the implementations of the required methods for the two new types and they also provide a look at how the two types can be used

[codecov-io]

Codecov Report

Merging #74 (f6572a1) into master (ab55fb9) will increase coverage by 1.73%.
The diff coverage is 69.02%.

@@            Coverage Diff             @@
##           master      #74      +/-   ##
==========================================
+ Coverage   49.44%   51.17%   +1.73%     
==========================================
  Files          57       57              
  Lines        3762     3875     +113     
==========================================
+ Hits         1860     1983     +123     
+ Misses       1902     1892      -10     

Impacted Files	Coverage Δ
linfa-clustering/src/appx_dbscan/algorithm.rs	28.57% <ø> (ø)
linfa-clustering/src/dbscan/algorithm.rs	50.00% <ø> (ø)
linfa-reduction/src/pca/algorithms.rs	0.00% <0.00%> (ø)
src/dataset/impl_records.rs	0.00% <ø> (ø)
src/dataset/impl_dataset.rs	36.48% <42.52%> (+24.31%)	⬆️
linfa-bayes/src/gaussian_nb.rs	57.03% <50.00%> (ø)
linfa-svm/src/classification.rs	64.05% <50.00%> (+1.72%)	⬆️
linfa-svm/src/regression.rs	44.59% <50.00%> (+7.09%)	⬆️
src/metrics_classification.rs	49.21% <60.00%> (-0.79%)	⬇️
linfa-kernel/src/lib.rs	24.73% <66.66%> (-0.27%)	⬇️
... and 24 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ab55fb9...f6572a1. Read the comment docs.

[bytesnake]

awesome! I'm currently traveling and will be back end of december, but the changes are looking good 👍 for `map_targets`: just drop the Vec implementation, it is only confusing for the user 20.12.2020 10:59:02 Ivano Donadi <notifications@github.com>:

…

@Sauro98 commented on this pull request. ---------------------------------------- In src/dataset/impl_dataset.rs[#74 (comment)]: > +    pub fn map_targets_array<T, G: FnMut(&E) -> T>(self, fnc: G) -> Dataset<F, T> { +        let DatasetBase { +            records, +            targets, +            weights, +            .. +        } = self; + +        let new_targets = targets.iter().map(fnc).collect::<Vec<T>>(); +        let new_targets = Array1::from_shape_vec(new_targets.len(), new_targets).unwrap(); + +        DatasetBase { +            records, +            targets: new_targets, +            weights, +        } +    } I had to add it in order to change the return type of functions in the datasets sub-crate, since map_targets puts the new targets in a Vec — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub[#74 (review)], or unsubscribe[https://github.com/notifications/unsubscribe-auth/AAHRRKI5C2RDGRWM7N4TBULSVXDGDANCNFSM4VC6ZXLQ]. [data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAEIAAABCCAYAAADjVADoAAAAAXNSR0IArs4c6QAAAARzQklUCAgICHwIZIgAAAAnSURBVHic7cEBDQAAAMKg909tDwcUAAAAAAAAAAAAAAAAAAAAAPBjRFIAASHKmKkAAAAASUVORK5CYII=###24x24:true###][Tracking-Bild][https://github.com/notifications/beacon/AAHRRKKOZXGBQ5YQ7F46BJTSVXDGDA5CNFSM4VC6ZXL2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOEER6Z7Q.gif]

[bytesnake]

yes! 20.12.2020 10:56:43 Ivano Donadi <notifications@github.com>:

…

@Sauro98 commented on this pull request. ---------------------------------------- In src/dataset/mod.rs[#74 (comment)]: > +        assert_eq!(train.targets().len(), 25); +        assert_eq!(val.targets().len(), 25); split_with_ratio_view for type Dataset uses the already existing implementation of the method, but this returns a slice as the targets. Would it be better to re-implement it for Dataset so that it returns an ArrayView1 as the targets? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub[#74 (review)], or unsubscribe[https://github.com/notifications/unsubscribe-auth/AAHRRKJD6PU6YUILRGMOPCTSVXC5NANCNFSM4VC6ZXLQ]. [data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAEIAAABCCAYAAADjVADoAAAAAXNSR0IArs4c6QAAAARzQklUCAgICHwIZIgAAAAnSURBVHic7cEBDQAAAMKg909tDwcUAAAAAAAAAAAAAAAAAAAAAPBjRFIAASHKmKkAAAAASUVORK5CYII=###24x24:true###][Tracking-Bild][https://github.com/notifications/beacon/AAHRRKLF5HIKMG3RO47QYELSVXC5NA5CNFSM4VC6ZXL2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOEER6YRA.gif]

[Sauro98]

Hi, I've added k-folding for Dataset and DatasetView types, implemented the changes mentioned in the previous comments and I've added a shy attempt at documentation. There are some questions I would like to ask:

• Is that style/content of documentation alright? If so, or even if you have some suggestions, I would gladly write some more for the impl_dataset.rs file
• I see that in general the methods in this file don't check for correctness of input parameters, and so I left the k parameter for k-folding unchecked. Is this alright? Or else, should methods instead return a Result catching any error with the inputs?

Of course, I'm here for any comments about the changes already implemented. Happy holidays ☃️

[bytesnake]

Hi, I've added k-folding for Dataset and DatasetView types, implemented the changes mentioned in the previous comments and I've added a shy attempt at documentation. There are some questions I would like to ask:

* Is that style/content of documentation alright? If so, or even if you have some suggestions, I would gladly write some more for the `impl_dataset.rs` file

They are looking good, I think that documentation should really be improved. You can take this blogpost as a guide how to document functions: https://deterministic.space/machine-readable-inline-markdown-code-cocumentation.html

* I see that in general the methods in this file don't check for correctness of input parameters, and so I left the `k` parameter for k-folding unchecked. Is this alright? Or else, should methods instead return a `Result` catching any error with the inputs?

yes we should, but only once we have proper error handling with thiserror

Of course, I'm here for any comments about the changes already implemented. Happy holidays snowman_with_snow

thanks :)

[bytesnake]

thanks for

• switching to DatasetBase
• extending testing and documentation
• introducing k-folding

[bytesnake]

I merged the naive bayes PR, can you rebase?

[bytesnake]

one idea which would improve readability:
add a field to DatasetBase called features: Vec<String> to give each feature a name. This could be used in printing correlation or decision tree etc.
But this would probably go into a follow up PR as these are already enough changes

[Sauro98]

did not mean to start a review