Skip to content

Release 0.3 - #235

Merged
Mec-iS merged 22 commits into
developmentfrom
release-0.3
Nov 8, 2022
Merged

Mec-iS merged 22 commits into
developmentfrom
release-0.3

Conversation

@Mec-iS

@Mec-iS Mec-iS commented Nov 7, 2022 •

Copy link
Copy Markdown
Collaborator

This is the release candidate for v0.3

Please take a look, run some tests, etc.

Changes compared to development:

  • improve documentation
  • make default feature empty (no optional dependencies) refactor feature system
  • cleanup unused variables

@Mec-iS
Mec-iS requested review from montanalow and morenol November 7, 2022 12:22
@Mec-iS
Mec-iS force-pushed the release-0.3 branch 2 times, most recently from 8480b5a to 1eb85a7 Compare November 7, 2022 12:32
@codecov-commenter

codecov-commenter commented Nov 7, 2022 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 54.54545% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 43.97%. Comparing base (aab3817) to head (4558be5).
⚠️ Report is 83 commits behind head on development.

Files with missing lines Patch % Lines
src/tree/decision_tree_classifier.rs 50.00% 2 Missing ⚠️
src/tree/decision_tree_regressor.rs 50.00% 2 Missing ⚠️
src/cluster/kmeans.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@               Coverage Diff               @@
##           development     #235      +/-   ##
===============================================
+ Coverage        43.93%   43.97%   +0.04%     
===============================================
  Files               85       85              
  Lines             7290     7281       -9     
===============================================
- Hits              3203     3202       -1     
+ Misses            4087     4079       -8     

☔ View full report in Codecov by Sentry.
📢 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.

@Mec-iS

Mec-iS commented Nov 7, 2022

Copy link
Copy Markdown
Collaborator Author

@morenol please take a look

we have to fix the complications with the random numbers range generator at #214

we have now a clearer setup for the features:

  • default: only base dependencies (no optional), should work with Wasi
  • serde: whatever requires serialization
  • datasets: whatever requires datasets
  • std
  • other

I don't understand the role of the std feature, I suppose is meant to use the standard library randomization library.
Now, we have to assure that:

  • Wasm/Wasi (default) use the right implementation for random (currently getrandom),
  • std feature uses the standard random
  • datasets is currently using rand_distr, is there the possibility to make it use std random or getrandom?

@Mec-iS

Mec-iS commented Nov 8, 2022 •

Copy link
Copy Markdown
Collaborator Author

Fixed!

Features are now:

[features]
default = []
serde = ["dep:serde"]
ndarray-bindings = ["dep:ndarray"]
datasets = ["dep:rand_distr", "std_rand", "serde"]
std_rand = ["rand/std_rng", "rand/std"]

Comment thread src/cluster/kmeans.rs Outdated
Comment thread src/ensemble/random_forest_classifier.rs Outdated
Comment thread CHANGELOG.md
Comment thread src/cluster/kmeans.rs
Comment thread Cargo.toml Outdated
num-traits = "0.2.12"
num = "0.4"
rand = { version = "0.8.5", default-features = false, features = ["small_rng"] }
getrandom = { version = "*", features = ["js"] }

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me check if there are issues with enabling js always.

@Mec-iS Mec-iS Nov 8, 2022 •

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need it right?
I assumed js is just an additional feature, shouldn't affect the base functionality

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need an alternative for this branch of the match

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, this feature flag only has effect for wasm32-unknown-unknown. Enabling this feature breakes the usage of wasm32-unknown-unknown with wasmtime and wasmer because it adds wasmbindgen to the build

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can avoid that branch by setting a seed for those targets. In any case I think that we could use get_random but only enabling "js" feature if it is a explicitly enabled feature

@morenol morenol Nov 8, 2022 •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, I think that we should keep this line in the features section:

js = ["getrandom/js"]

and remove js from being enabled by default in this line

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my bad it seems to not be working with this change

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move all the changes related to rand vs getrandom to another PR? so we can merge the other changes?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Mec-iS I think that we shouldnt merge this PR

Comment thread Cargo.toml Outdated
@Mec-iS
Mec-iS merged commit 161d249 into development Nov 8, 2022
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.

3 participants