Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement a generic read_csv method #147

Merged
merged 3 commits into from
Sep 19, 2022
Merged

Conversation

titoeb
Copy link
Collaborator

@titoeb titoeb commented Sep 2, 2022

Looking into implementing #66.

@titoeb titoeb changed the title Implement a generic csv method [#66](https://github.com/smartcorelib/smartcore/issues/66) Implement a generic read_csv method Sep 2, 2022
@codecov-commenter
Copy link

codecov-commenter commented Sep 2, 2022

Codecov Report

Merging #147 (10f18ea) into development (d305406) will increase coverage by 0.36%.
The diff coverage is 93.36%.

@@               Coverage Diff               @@
##           development     #147      +/-   ##
===============================================
+ Coverage        83.90%   84.26%   +0.36%     
===============================================
  Files               81       84       +3     
  Lines             8724     8989     +265     
===============================================
+ Hits              7320     7575     +255     
- Misses            1404     1414      +10     
Impacted Files Coverage Δ
src/readers/io_testing.rs 84.44% <84.44%> (ø)
src/readers/error.rs 94.11% <94.11%> (ø)
src/linalg/mod.rs 62.63% <95.23%> (+4.05%) ⬆️
src/readers/csv.rs 95.68% <95.68%> (ø)
src/math/num.rs 82.69% <100.00%> (-1.40%) ⬇️
src/algorithm/neighbour/cover_tree.rs 84.29% <0.00%> (-0.42%) ⬇️
src/preprocessing/numerical.rs 89.62% <0.00%> (+0.74%) ⬆️
src/metrics/recall.rs 92.50% <0.00%> (+18.42%) ⬆️
src/metrics/precision.rs 92.50% <0.00%> (+18.42%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@titoeb titoeb force-pushed the read-csv branch 2 times, most recently from b354ae3 to 9310790 Compare September 2, 2022 06:52
@Mec-iS
Copy link
Collaborator

Mec-iS commented Sep 2, 2022

hey. Thanks again for contributing.

Some notes:

  • take a look to the warnings and linting, always run cargo fmt --all -- --check and cargo clippy --all-features -- -Drust-2018-idioms -Dwarnings.
  • always add a dosctring on top of a new module using //! when you explain the content and objectives for the module: in this case for example
//! Load a `Matrix` from CSV file
//! * ...
//! * only accepting commas as separators
//! ...
//! Example:
//! ```
//! ...
//! matrix_from_csv::<f64, Vec<_>, DenseMatrix<_>>(
                    test_csv_file
                        .expect("Temporary file could not be written.")
                        .path()
                )
//!

"Example" part is important as code snippets are also compiled and used as tests.
The best examples of documentation are in files like src/linalg/cholesky.rs, we would like to stick to that standard.

  • Adding the FromStr trait to RealNumber is for sure the rustucean way of doing it 👍🏼 maybe adding a test for RealNumber::from_str in src/math/num.rs would be an additional control, the more tests the better
  • The convention for generic type in Rust and in this crate is T (not Type)

Great work, especially with the test coverage and error handling 💯

@Mec-iS Mec-iS self-requested a review September 2, 2022 09:34
@titoeb
Copy link
Collaborator Author

titoeb commented Sep 2, 2022

hey. Thanks again for contributing.

Some notes:

  • take a look to the warnings and linting, always run cargo fmt --all -- --check and cargo clippy --all-features -- -Drust-2018-idioms -Dwarnings.
  • always add a dosctring on top of a new module using //! when you explain the content and objectives for the module: in this case for example
//! Load a `Matrix` from CSV file
//! * ...
//! * only accepting commas as separators
//! ...
//! Example:
//! ```
//! ...
//! matrix_from_csv::<f64, Vec<_>, DenseMatrix<_>>(
                    test_csv_file
                        .expect("Temporary file could not be written.")
                        .path()
                )
//!

"Example" part is important as code snippets are also compiled and used as tests. The best examples of documentation are in files like src/linalg/cholesky.rs, we would like to stick to that standard.

  • Adding the FromStr trait to RealNumber is for sure the rustucean way of doing it 👍🏼 maybe adding a test for RealNumber::from_str in src/math/num.rs would be an additional control, the more tests the better
  • The convention for generic type in Rust and in this crate is T (not Type)

Great work, especially with the test coverage and error handling 💯

Hi @Mec-iS !
Thanks for already looking into this, your comments make sense 👍 This is still a Draft PR so there is still a lot of work for me to do until it is ready to opened. Feel free to continue to comment on the progress of course, but also take into consideration that the implementation will probably change quite alot until then. I mainly opened the PR to make visible that I work on the issue and give you already a sense of the direction in which I am heading, I am very happy that you are overall like the changes.

@titoeb titoeb force-pushed the read-csv branch 3 times, most recently from 5591bff to 23e826f Compare September 12, 2022 06:44
To construct a `Matrix` from csv, and therefore from string, I need to be able to deserialize a generic `RealNumber` from string.
@titoeb titoeb force-pushed the read-csv branch 4 times, most recently from 6c8e5e8 to ddfa5f0 Compare September 19, 2022 06:20
@titoeb titoeb marked this pull request as ready for review September 19, 2022 06:39
@titoeb
Copy link
Collaborator Author

titoeb commented Sep 19, 2022

hey. Thanks again for contributing.

Some notes:

  • take a look to the warnings and linting, always run cargo fmt --all -- --check and cargo clippy --all-features -- -Drust-2018-idioms -Dwarnings.
  • always add a dosctring on top of a new module using //! when you explain the content and objectives for the module: in this case for example
//! Load a `Matrix` from CSV file
//! * ...
//! * only accepting commas as separators
//! ...
//! Example:
//! ```
//! ...
//! matrix_from_csv::<f64, Vec<_>, DenseMatrix<_>>(
                    test_csv_file
                        .expect("Temporary file could not be written.")
                        .path()
                )
//!

"Example" part is important as code snippets are also compiled and used as tests. The best examples of documentation are in files like src/linalg/cholesky.rs, we would like to stick to that standard.

  • Adding the FromStr trait to RealNumber is for sure the rustucean way of doing it 👍🏼 maybe adding a test for RealNumber::from_str in src/math/num.rs would be an additional control, the more tests the better
  • The convention for generic type in Rust and in this crate is T (not Type)

Great work, especially with the test coverage and error handling 100

  • Linting now runs through
  • I added docstrings when it was applicable.
  • I replaced Type with T.

@Mec-iS Mec-iS merged commit b6f585e into smartcorelib:development Sep 19, 2022
@titoeb titoeb deleted the read-csv branch September 19, 2022 12:53
morenol pushed a commit that referenced this pull request Nov 8, 2022
* feat: Add interface to build `Matrix` from rows.
* feat: Add option to derive `RealNumber` from string.
To construct a `Matrix` from csv, and therefore from string, I need to be able to deserialize a generic `RealNumber` from string.
* feat: Implement `Matrix::read_csv`.
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.

None yet

3 participants