-
Notifications
You must be signed in to change notification settings - Fork 25
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
Argmin argmax skipnan #33
Argmin argmax skipnan #33
Conversation
Considering the discussion we are having in #32 I think it makes sense to align this PR to it as well 👍 |
Cool thanks @LukeMathWalker I have updated the docs to reflect the changes. Do you guys want to update from |
Looks like with |
I don't think, right now, there is a performance difference between a Could you add a property test for both functions similar to https://github.com/jturner314/ndarray-stats/blob/7df07289c3a378154be51642e7ce610312b286f0/tests/quantile.rs#L28? |
Hey mate, I have made the returns clearer and added |
Also switch to use |
It looks good to me. I'll let it open another day to give @jturner314 the opportunity to chip in if he has any observation. Good work @phungleson! |
Cool thanks @LukeMathWalker let me know if you want to convert On a separate random thought, if |
The old tests were incorrect because `min`/`max` return `None` when there are *any* NaN values (or the array is empty), while `argmin/max_skipnan` should return `None` only when *all* the values are NaNs (or the array is empty). This wasn't caught earlier because the `quickcheck::Arbitrary` implementation for `f32` generates only finite values. To make sure the behavior with NaN values is properly tested, the element type in the test has been changed to `Option<i32>`.
Thanks for working on this @phungleson! I think it would be nice to abstract out some of the logic with an Edit: Whoops, it looks like I added back the
That's correct.
You seem to be asking about iteration order in relation to memory layout. |
Improvements to argmin/max_skipnan
@jturner314 Thanks for the explanation, all good! The implementation is also very good, thanks! BTW, PR is probably not a right place to ask this (if it is inappropriate here that is all good too thanks!) but my head is still hurt with the lifetime here, it would be great if you have time for quick explanation or point me to some source to read. Does this mean? fn indexed_fold_skipnan<'a, F, B>(&'a self, init: B, f: F) -> B
where
A: 'a,
F: FnMut(B, (D::Pattern, &'a A::NotNan)) -> B; |
Yes.
Yes, that is one of the reasons, but somewhat indirectly. First, it's helpful to understand what exactly it means for a type to live at least as long as a lifetime. Most types, such as It's also worth pointing out that If we remove the
so the problem is that without the How does adding an Fwiw, this is getting pretty into-the-weeds of lifetimes. I didn't think about this when I was writing the function. If we just follow the compiler's suggestions, starting from no bounds: fn indexed_fold_skipnan<'a, F, B>(&'a self, init: B, f: F) -> B
where
F: FnMut(B, (D::Pattern, &'a A::NotNan)) -> B; the compiler says that fn indexed_fold_skipnan<'a, F, B>(&'a self, init: B, f: F) -> B
where
A::NotNan: 'a,
F: FnMut(B, (D::Pattern, &'a A::NotNan)) -> B; then the compiler says that fn indexed_fold_skipnan<'a, F, B>(&'a self, init: B, f: F) -> B
where
A: 'a,
A::NotNan: 'a,
F: FnMut(B, (D::Pattern, &'a A::NotNan)) -> B; This then satisfies the compiler, and we could stop here. We could try removing the
I can't think of a situation where On another note, it would be possible to define fn indexed_fold_skipnan<F, B>(&self, init: B, f: F) -> B
where
F: FnMut(B, (D::Pattern, &A::NotNan)) -> B; but that's not as convenient for the caller because they get ¹ Technical note: It turns out that this observation (associated types can never have a shorter lifetime than |
I've added a "Breaking changes" label because adding a method to |
Thanks for your awesome explanation! @jturner314 😄 It probably will take some more time to have the concept I will probably read more about |
You're welcome. I recommend reading the book if you haven't already; it's very good. The concept of lifetimes exists in other languages too; they just aren't explicit and the compiler doesn't provide support for getting them correct. Rust's innovation is that the language provides explicit support for dealing with lifetimes, and compiler checks them for correctness. For example, I've worked on C++ projects in the past, and it wasn't uncommon to talk about lifetimes and ownership, because trying to access an object in C++ after it's been freed is undefined behavior. For example, the docs for Many languages work around lifetime issues by using things like reference counting (e.g. The concept of a "type with a lifetime" exists in other languages too. For example, a |
Thanks a lot @jturner314 your explanation is very clear, I have read them a few times and each time things seem clearer and clearer. It is also easier to understand when I read the book now. Thanks! |
/// returning the resulting value. | ||
/// | ||
/// Elements are visited in arbitrary order. | ||
fn indexed_fold_skipnan<'a, F, B>(&'a self, init: B, f: F) -> B |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, I wonder if it is better if we introduce indexed_iter_skipnan
instead of indexed_fold_skipnan
so that it can be used for different purposes, i.e. indexed_iter_skipnan().fold()
is one of the usecases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't mind adding indexed_iter_skipnan()
in addition to indexed_fold_skipnan()
, but the semantics of indexed_iter_skipnan().fold()
and indexed_fold_skipnan()
are a bit different. (I'd assume that indexed_iter_skipnan()
would iterate in standard order to be consistent with indexed_iter()
, while I wouldn't assume that for indexed_fold_skipnan()
.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ok, I forgot about the order semantic, in that case, perhaps indexed_iter_skipnan
can be added later when it has actual usecase, wdyt? is there anything you want me to look into before merging this?
Anything left to consider for this one @jturner314 Thanks! |
Looks good to me. Thanks for working on this! |
Implement
argmin_skipnan
andargmax_skipnan