rust-ml / linfa

A Rust machine learning framework.
Apache License 2.0
3.6k stars 232 forks source link

Documentation maintenance and small fixes #82

Closed Sauro98 closed 3 years ago

Sauro98 commented 3 years ago

Here is a collection of work that needs to be done regarding documentation and minor issues with the code itself. Picking an item from this list can be a good way to start getting acquainted with the codebase and provide a useful contribution. 😄 👍🏻

In general, I would say that the pages for the individual algorithms in the linfa-clustering and linfa-elasticnet sub-crates can be good references for the structure of a documentation page.

It's suggested to look at the existing documentation in your local build rather than on doc.rs since some pages may have already been updated.

quietlychris commented 3 years ago

I'll be starting work on some of these tonight!

quietlychris commented 3 years ago

Apologies for the delay--I've been having some issues with building the linfa-bayes sub-crate, due to an issue with the rand::rngs::SmallRng import. Not exactly sure why it's happening yet, but I've experienced the same thing on both aarch64 and x86-64 architectures, using compilers rustc 1.149.0-stable and rustc 1.51.0-nightly respectively, after cloning a fresh repository from master. Can anyone else check if they're seeing the same behaviour?

Edit: Never mind, it's only happening when I try running cargo build directly in the linfa-bayes sub-directory, not when I run cargo build --all in the main one. I thought I might be able to build the documentation in each sub-crate separately, but not a huge deal to work from the main library.

Sauro98 commented 3 years ago

Can confirm, the same thing is happening to me

bytesnake commented 3 years ago

I pushed the missing feature flag in a4362fa, do you have any idea why it compiles even without the proper feature flag. The error actually slipped through our CI system :/

quietlychris commented 3 years ago

I'm not exactly sure. When I run the cargo build --all, linfa_bayes is listed as being built alongside the others. Maybe it has something to do with the dependency not being available from the higher-level crate where it's being called from (rand is not an explicit dependency in linfa_bayes's Cargo.toml file)?

I'm also seeing the same behaviour in linfa-kernel, where the rand::rngs::SmallRng is causing a compilation error, but not in any of the other sub-crates.

relf commented 3 years ago

Hi. For what it's worth, I have no problem with cargo build --all on CentOS7, stable, rustc 1.49, but on Windows 7, stable-msvc, rustc 1.49 I got the following error when compiling linfa-reduction:

error[E0432]: unresolved imports `ndarray_linalg::lobpcg`, `ndarray_linalg::lobpcg`, `ndarray_linalg::TruncatedOrder`
 --> linfa-reduction\src\diffusion_map\algorithms.rs:3:21
  |
3 |     eigh::EighInto, lobpcg, lobpcg::LobpcgResult, Lapack, Scalar, TruncatedOrder, UPLO,
  |                     ^^^^^^  ^^^^^^                                ^^^^^^^^^^^^^^ no `TruncatedOrder` in the root
  |                     |       |
  |                     |       could not find `lobpcg` in `ndarray_linalg`
  |                     no `lobpcg` in the root

error[E0432]: unresolved imports `ndarray_linalg::TruncatedOrder`, `ndarray_linalg::TruncatedSvd`
 --> linfa-reduction\src\pca\algorithms.rs:6:22
  |
6 | use ndarray_linalg::{TruncatedOrder, TruncatedSvd};
  |                      ^^^^^^^^^^^^^^  ^^^^^^^^^^^^ no `TruncatedSvd` in the root
  |                      |
  |                      no `TruncatedOrder` in the root

Not sure it is on linfa. It seems I miss some ndarray-linalg parts. Any idea?

bytesnake commented 3 years ago

mh strange are you using the current version of ndarray-linalg 0.12.1? Try to perform a cargo update and see whether the dependency is updated

Sauro98 commented 3 years ago

It works for me on windows 10, rustc 1.49 stable-x86_64-pc-windows-msvc, both when building the whole workspace and when building linfa-reduction by itself. Edit: well, the build itself works but then actually running it is a different story since it gives me a bunch of problems with BLAS (AMD processor if that is of any help)

relf commented 3 years ago

cargo update did the job! It works now. cargo build --all should have handled that or did I miss something?

bytesnake commented 3 years ago

@relf no we should fix the ndarray-linalg version to 0.12.1 on the linfa side (done in cf3c611429d)

@quietlychris I found the reason for your error: cargo build --all seems to take the development dependencies into consideration as well and ndarray-rand uses the small_rng feature flag which got enabled for all sub-crates. linfa-bayes on the other hand does not use ndarray-rand and consequently misses the feature flag.

relf commented 3 years ago

@relf no we should fix the ndarray-linalg version to 0.12.1 on the linfa side

Yep you nailed it, from cargo update output, indeed:

ndarray-linalg v0.12.0 -> v0.12.1
Sauro98 commented 3 years ago

Posting here under the "small fixes" label: on mobile the code snippets in the website exit the code box on the right side

bytesnake commented 3 years ago

with #96 all issues are resolved, but the line break in the website's code box is still smoking

bytesnake commented 3 years ago

fixed the issue with an overflowing code box in 402668d :crab: