feat!: Update `strum` and `ordered-float` dependencies and change From<LogProb> into TryFrom<LogProb> for NotNan<f64>. #491

evolvedmicrobe · 2022-05-04T01:44:30Z

Figure it'll need to change eventually.

Figure we'll need to change eventually.

coveralls · 2022-05-04T01:52:05Z

Coverage remained the same at 88.39% when pulling 99f2ba8 on evolvedmicrobe:nd/update_strum into 976e27d on rust-bio:master.

ordered-float ditched their From<f64> methods for NotNan and instead made them TryFrom<f64> (reem/rust-ordered-float@39f76bb). This change makes sense because From trait implementations generally shouldn’t panic, which the implementation did if the f64 was a NaN, and so TryFrom is more appropriate. This commit accounts for this change in the upgrade

`ndarray` has renamed `genrows()` to `rows()`

adam-azarchs · 2022-05-31T17:31:34Z

src/stats/probs/mod.rs

@@ -403,7 +403,7 @@ impl From<NotNan<f64>> for LogProb {

 impl From<LogProb> for NotNan<f64> {


Should probably also add a TryFrom impl here for people who want to acknowledge the fact that the conversion is fallible.

Hmm, that's more of a larger change, but would be consistent with the reasoning in unordered-float

src/stats/probs/adaptive_integration.rs

adam-azarchs · 2022-05-31T17:36:23Z

src/stats/probs/adaptive_integration.rs

@@ -43,7 +43,7 @@ use ordered_float::NotNan;
 /// );
 /// abs_diff_eq!(integral.exp(), 0.682, epsilon=0.01);
 /// ```
-pub fn ln_integrate_exp<T, F>(
+pub fn ln_integrate_exp<T, F, E>(


I think a PR changing the signature of a public method like this (which, granted, in most cases won't actually affect most users because the type will just be inferred, but will affect anyone who needs to specify any of the generic parameters) should probably not be labeled "chore", since "chore" PRs don't show up in the changelog.

Would you classify it at a build: then?

I think it should be feat: actually, since the API is extended with a new feature here (allowing to have a fallible conversion from f64)

adam-azarchs · 2022-05-31T19:20:50Z

src/stats/probs/mod.rs

-impl From<LogProb> for NotNan<f64> {
-    fn from(p: LogProb) -> NotNan<f64> {
-        NotNan::new(*p).unwrap()
+impl TryFrom<LogProb> for NotNan<f64> {


Sorry I was unclear but I would actually probably recommend leaving the (potentially-panicing) From implementation for now in order to avoid breaking compatibility, but mark it as deprecated.

unfortunately you don't seem to be able to implement a deprecated From and a non-deprecated TryFrom, as the blanket implementation (https://github.com/rust-lang/rust/blob/3e525e3f6d9e85d54fa4c49b52df85aa0c990100/src/libcore/convert.rs#L580) leads to the compiler thinking you've implemented TryFrom already and so it won't compile.

I prefer the tryfrom here, let's make this a breaking change (feat!:), then.

evolvedmicrobe

Looks like packed_simd is broken

src/stats/probs/adaptive_integration.rs

evolvedmicrobe · 2022-05-31T18:52:12Z

src/stats/probs/mod.rs

@@ -403,7 +403,7 @@ impl From<NotNan<f64>> for LogProb {

 impl From<LogProb> for NotNan<f64> {


Hmm, that's more of a larger change, but would be consistent with the reasoning in unordered-float

evolvedmicrobe · 2022-05-31T20:47:51Z

src/stats/probs/mod.rs

-impl From<LogProb> for NotNan<f64> {
-    fn from(p: LogProb) -> NotNan<f64> {
-        NotNan::new(*p).unwrap()
+impl TryFrom<LogProb> for NotNan<f64> {


unfortunately you don't seem to be able to implement a deprecated From and a non-deprecated TryFrom, as the blanket implementation (https://github.com/rust-lang/rust/blob/3e525e3f6d9e85d54fa4c49b52df85aa0c990100/src/libcore/convert.rs#L580) leads to the compiler thinking you've implemented TryFrom already and so it won't compile.

evolvedmicrobe · 2022-05-31T20:52:43Z

src/stats/probs/adaptive_integration.rs

@@ -43,7 +43,7 @@ use ordered_float::NotNan;
 /// );
 /// abs_diff_eq!(integral.exp(), 0.682, epsilon=0.01);
 /// ```
-pub fn ln_integrate_exp<T, F>(
+pub fn ln_integrate_exp<T, F, E>(


Would you classify it at a build: then?

The conversion from LogProb is not guaranteed to succeed, so we use `TryFrom` instead of `From`, as per the Rust docs: ```Note: The From trait must not fail. The From trait is intended for perfect conversions. If the conversion can fail or is not perfect, use TryFrom.``` Note this does change the API.

pmarks · 2022-06-01T21:04:31Z

@johanneskoester should probably take a look - I can chime in if he's not able to review in the next couple days.

Cargo.toml

adam-azarchs · 2022-06-01T21:33:48Z

Cargo.toml

@@ -37,16 +37,16 @@ serde_derive = "1.0"
 approx = ">=0.3, <0.6"
 custom_derive = "0.1"
 newtype_derive = "0.1"
-ordered-float = "1.0"
+ordered-float = "2.0"


Once we're updating this... would it be possible to make this

Suggested change

ordered-float = "2.0"

ordered-float = ">=2, <4"

There were breaking changes in 3.0.0, but if they don't break this crate then it would be preferable to allow users to decide which version they want.

Sounds reasonable to me. @adam-azarchs are you sure that the breaking changes don't affect us here?

Not 100%. They don't look like behavior changes, just some dropped implementations, so it should be easy enough to just try it and see if it compiles. I know for our internal purposes we're not going to be updating to 3.0 particularly soon but future proofing is good.

johanneskoester · 2022-06-02T07:45:48Z

Cargo.toml

@@ -37,16 +37,16 @@ serde_derive = "1.0"
 approx = ">=0.3, <0.6"
 custom_derive = "0.1"
 newtype_derive = "0.1"
-ordered-float = "1.0"
+ordered-float = "2.0"


Sounds reasonable to me. @adam-azarchs are you sure that the breaking changes don't affect us here?

Cargo.toml

johanneskoester · 2022-06-02T07:50:37Z

src/stats/probs/adaptive_integration.rs

@@ -43,7 +43,7 @@ use ordered_float::NotNan;
 /// );
 /// abs_diff_eq!(integral.exp(), 0.682, epsilon=0.01);
 /// ```
-pub fn ln_integrate_exp<T, F>(
+pub fn ln_integrate_exp<T, F, E>(


I think it should be feat: actually, since the API is extended with a new feature here (allowing to have a fallible conversion from f64)

johanneskoester · 2022-06-02T07:53:24Z

src/stats/probs/mod.rs

-impl From<LogProb> for NotNan<f64> {
-    fn from(p: LogProb) -> NotNan<f64> {
-        NotNan::new(*p).unwrap()
+impl TryFrom<LogProb> for NotNan<f64> {


I prefer the tryfrom here, let's make this a breaking change (feat!:), then.

Co-authored-by: Adam Azarchs <adam.azarchs@10xgenomics.com>

Cargo.toml

adam-azarchs · 2022-08-19T18:14:31Z

The MSRV failure here has nothing to do with this PR (or, in fact with any changes in this repo). I have a fix for the MSRV build in another PR: #497

Update strum crates

2f1b29e

Figure we'll need to change eventually.

evolvedmicrobe force-pushed the nd/update_strum branch from 3081213 to 2f1b29e Compare May 4, 2022 01:45

evolvedmicrobe changed the title ~~Update strum crates~~ chore: Update strum crates May 4, 2022

evolvedmicrobe mentioned this pull request May 4, 2022

build: Update Strum to Latest Version rust-bio/rust-bio-types#37

Merged

evolvedmicrobe added 3 commits May 31, 2022 10:15

Remove Deprecated Function Call

a42e59d

`ndarray` has renamed `genrows()` to `rows()`

Formatting Fix

c30ce2f

evolvedmicrobe changed the title ~~chore: Update strum crates~~ chore: Update strum and ordered-float crates May 31, 2022

Update CHANGELOG

080b9f7

adam-azarchs reviewed May 31, 2022

View reviewed changes

evolvedmicrobe force-pushed the nd/update_strum branch from 643b09a to c036645 Compare May 31, 2022 19:50

evolvedmicrobe commented May 31, 2022

View reviewed changes

evolvedmicrobe force-pushed the nd/update_strum branch from c036645 to 096e156 Compare June 1, 2022 20:41

pmarks requested a review from johanneskoester June 1, 2022 21:03

adam-azarchs reviewed Jun 1, 2022

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

adam-azarchs reviewed Jun 1, 2022

View reviewed changes

johanneskoester requested changes Jun 2, 2022

View reviewed changes

evolvedmicrobe changed the title ~~chore: Update strum and ordered-float crates~~ feat!: Update strum and ordered-float crates Aug 11, 2022

Apply suggestions from code review

aaffd64

Co-authored-by: Adam Azarchs <adam.azarchs@10xgenomics.com>

adam-azarchs reviewed Aug 11, 2022

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

johanneskoester changed the title ~~feat!: Update strum and ordered-float crates~~ feat!: Update strum and ordered-float dependencies and change From<LogProb> into TryFrom<LogProb> for NotNan<f64>. Aug 17, 2022

johanneskoester added 2 commits August 17, 2022 15:56

Update CHANGELOG.md

0ad0658

Update Cargo.toml

c52604d

johanneskoester approved these changes Aug 17, 2022

View reviewed changes

Fix build issue.

aa90c4e

adam-azarchs force-pushed the nd/update_strum branch from 35c1c94 to aa90c4e Compare August 19, 2022 17:53

Merge 'master' into nd/update_strum

99f2ba8

pmarks merged commit 57ccf8f into rust-bio:master Aug 19, 2022

github-actions bot mentioned this pull request Aug 19, 2022

chore: release 1.0.0 #498

Closed

adam-azarchs deleted the nd/update_strum branch August 20, 2022 00:25

github-actions bot mentioned this pull request Aug 30, 2022

chore: release 0.42.0 #500

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: Update `strum` and `ordered-float` dependencies and change From<LogProb> into TryFrom<LogProb> for NotNan<f64>. #491

feat!: Update `strum` and `ordered-float` dependencies and change From<LogProb> into TryFrom<LogProb> for NotNan<f64>. #491

evolvedmicrobe commented May 4, 2022

coveralls commented May 4, 2022 •

edited

adam-azarchs May 31, 2022

evolvedmicrobe May 31, 2022

adam-azarchs May 31, 2022

evolvedmicrobe May 31, 2022

johanneskoester Jun 2, 2022

adam-azarchs May 31, 2022

evolvedmicrobe May 31, 2022

johanneskoester Jun 2, 2022

evolvedmicrobe left a comment

evolvedmicrobe May 31, 2022

evolvedmicrobe May 31, 2022

evolvedmicrobe May 31, 2022

pmarks commented Jun 1, 2022

adam-azarchs Jun 1, 2022

johanneskoester Jun 2, 2022

adam-azarchs Jun 2, 2022

johanneskoester Jun 2, 2022

johanneskoester Jun 2, 2022

johanneskoester Jun 2, 2022

adam-azarchs commented Aug 19, 2022

		@@ -403,7 +403,7 @@ impl From<NotNan<f64>> for LogProb {

		impl From<LogProb> for NotNan<f64> {

feat!: Update strum and ordered-float dependencies and change From<LogProb> into TryFrom<LogProb> for NotNan<f64>. #491

feat!: Update strum and ordered-float dependencies and change From<LogProb> into TryFrom<LogProb> for NotNan<f64>. #491

Conversation

evolvedmicrobe commented May 4, 2022

coveralls commented May 4, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evolvedmicrobe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmarks commented Jun 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adam-azarchs commented Aug 19, 2022

feat!: Update `strum` and `ordered-float` dependencies and change From<LogProb> into TryFrom<LogProb> for NotNan<f64>. #491

feat!: Update `strum` and `ordered-float` dependencies and change From<LogProb> into TryFrom<LogProb> for NotNan<f64>. #491

coveralls commented May 4, 2022 •

edited