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

Clip #470

Open
nilgoyette opened this issue Jun 29, 2018 · 7 comments
Open

Clip #470

nilgoyette opened this issue Jun 29, 2018 · 7 comments

Comments

@nilgoyette
Copy link
Collaborator

I can't find a numpy::clip function in ndarray. Is there one?

If not, I know I can use something like

arr.mapv(|v| if v < 4.0 { 0.0 } else { v } );

but do you think it would be better to have a clip function?

// min and max in the same function
arr.clip(Some(0.0), None);

// or 3 functions
arr.clip_min(0.0);
arr.clip_max(1.0);
arr.clip(0.0, 1.0);

I can code it, I just wonder if we want it.

@minierolls
Copy link

Just wanted to comment saying yes, please, I would love that! It's a lot more concise and easier to read.

@LukeMathWalker
Copy link
Member

It's an easy win, if we want to implement one. What do you think @jturner314 @max-sixty @termoshtt?

@max-sixty
Copy link
Contributor

max-sixty commented Aug 14, 2019

I think so! The python ecosystem have clip_lower (rather than clip_min), which I think is clearer.

No strong view on one vs two methods (or even both options). Rather than taking Option, you could ask people to pass inf / max_size, but less user-friendly.

@minierolls
Copy link

Would this mainly wrap mapv if implemented? I am interested in getting started contributing to ndarray and wondering if there might be any other magic involved in implementing this

@jturner314
Copy link
Member

Using what's currently available, we have this:

let arr = array![-2., -1., 0., 1., 2., f64::NAN];

// Clip lower, removing NANs. This can alternatively be written as
// `v.max(-1.)`. For types implementing `Ord`, you could use `cmp::max` or
// the `.max()` method.
let clip_lower_remove = arr.mapv(|v| f64::max(v, -1.));
println!("{}", clip_lower_remove);

// Clip lower, preserving NANs. This can alternatively be written like
// `num::clamp(v, -1., f64::INFINITY)`.
let clip_lower_preserve = arr.mapv(|v| if v < -1. { -1. } else { v });
println!("{}", clip_lower_preserve);

// Clip upper, removing NANs. This can alternatively be written as
// `v.min(1.)`. For types implementing `Ord`, you could use `cmp::min` or
// the `.min()` method.
let clip_upper_remove = arr.mapv(|v| f64::min(v, 1.));
println!("{}", clip_upper_remove);

// Clip upper, preserving NANs. This can alternatively be written like
// `num::clamp(v, f64::NEG_INFINITY, 1.)`.
let clip_upper_preserve = arr.mapv(|v| if v > 1. { 1. } else { v });
println!("{}", clip_upper_preserve);

// Clip, removing NANs. This case is weird, because it's unclear if the
// lower or upper bound should be returned in the NAN case.
let clip_remove = arr.mapv(|v| v.max(-1.).min(1.));
println!("{}", clip_remove);

// Clip, preserving NANs. There are also nightly-only `.clamp()` methods
// in `std` for `f32`, `f64`, and `Ord` types.
let clip_preserve = arr.mapv(|v| num::clamp(v, -1., 1.));
println!("{}", clip_preserve);

In the most cases, I think it makes sense to preserve NANs. The double-sided clip using mapv with num::clamp is actually rather nice, but the one-sided clips aren't pretty (unless you want to remove NANs, which I think is the rare case).

ndarray provides only map/mapv/mapv_inplace/etc. for other elementwise operations, so I think it makes the most sense rely on them for clipping as well (instead of adding new clip/clip_lower/etc. methods). Using map/mapv with num::clamp is pretty nice. Maybe we could convince the num maintainers to add clamp_lower and clamp_upper functions to clean up the one-sided clips?

If someone wanted to avoid using map and related methods, they could write an extension trait to add methods like clip, clip_inplace, clip_move, exp, exp_inplace, exp_move, etc., to ArrayBase.

@termoshtt
Copy link
Member

FYI: This table shows how each language/library implements clamp.
rust-lang/rfcs#1961 (comment)

Clip, preserving NANs. There are also nightly-only .clamp() methods

This seems to take a long time for stabilization. We should be consistent with it after its stabilization. rust-lang/rust#44095

Maybe we could convince the num maintainers to add clamp_lower and clamp_upper functions to clean up the one-sided clips?

This sounds better.

bors bot added a commit to rust-num/num-traits that referenced this issue Sep 14, 2019
122: NAN preserving clamp_lower/upper r=cuviper a=termoshtt

`NAN` preserving lower- and upper-clamp.

Cc: rust-ndarray/ndarray#470 (comment)

Co-authored-by: Toshiki Teramura <[email protected]>
@ZuseZ4
Copy link
Contributor

ZuseZ4 commented Nov 28, 2020

Just giving this a push, since clamp will become available in Rust 1.50
rust-lang/rust#77872

I would vote for directly offering these methods as part of ndarray, since it's a common feature in other langues (python's ndarray, Torch,..).

I guess it makes sense to have clamp(..), clamp_min() and clamp_max().
Is there another function which we should add along the way?

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

No branches or pull requests

7 participants