I'm rewriting some code I have in Python in Rust. Python has a min(collection, [key]) function which I can use to find the item which has a min value of a particular element in a tuple. I don't see an equivalent in Rust which I think I can understand why. I've come up with this solution. It works fine but I'm wondering what refinements can be made to it if any. Thanks.
#[derive(Clone, Debug)]
struct Data(u32, f32, f32); // time, temp, press
impl Data {
fn get_temprature(&self) -> f32 {
self.1
}
fn _get_pressure(&self) -> f32 {
self.2
}
fn min(data: &[Data], f: fn(&Data) -> f32) -> Data {
data.iter()
.reduce(|a, b| if f(a) < f(b) { a } else { b })
.unwrap_or(&Data(0, f32::MAX, f32::MAX))
.clone()
}
fn max(data: &[Data], f: fn(&Data) -> f32) -> Data {
data.iter()
.reduce(|a, b| if f(a) > f(b) { a } else { b })
.unwrap_or(&Data(0, f32::MIN, f32::MIN))
.clone()
}
}
fn main() {
let v: Vec<Data> = vec![
Data(10, 75.0, 10000.0),
Data(11, 100.0, 12000.0),
Data(12, 25.0, 8000.0),
Data(13, 50.0, 9000.0),
];
let tn = Data::min(&v, Data::get_temprature);
println!("min: {}", tn.get_temprature());
let tx = Data::max(&v, Data::get_temprature);
println!("max: {}", tx.get_temprature());
}
Edit: Oh I see you're returning f32 which is not Ord. It's because having NaN makes the whole comparison doesn't make sense. If you know it doesn't have any NaN, wrap the float with NotNan.
The biggest thing that pops out to me is the lack of newtypes for time, temperature, and pressure. In Rust, newtypes are pretty much free in terms of time and space consumed, and not doing so has been a 125 million dollar mistake in the past for NASA.
It's common to have a relaxed and general version with Ordering callback besides the handy methods with Ord bound. And the float types in Rust are also provided with an inherent Ordering method which may or may not be what you want though.
In your case instead of having a comment describing what the tuple elements represent, I think you should use a normal struct.
struct Data {
time: u32,
temp: f32,
pressure: f32,
}
The reason being, as your code is written you have to remember which order your fields are in. Easy to make a mistake that way. You could accidently use the temperature field when you meant to use pressure.
That is a good idea. I've already made that mistake when refactoring some existing Python and Javascript code. Thanks for the link an interesting read!
Thank you. I've already made this mistake in Python. Part of the reason I did it this way is I'm de-serializing a Json request which has this format. I'll see if I can adapt something like this and get my Json deserialize code to work again.
Thanks. I did not scour the docs enough. This is the kind of thing I was looking for. I've hit a few issues. I think a reply later anticipates them. It was useful none the less to figure out how to do my own version.
Oh, yeah, that can be a problem when you're interacting with json data. Sounds like you may not have control over that part. One thing you could do is have a type that represents parsed json data, and then implement From for that type to convert into a more idiomatic type. It is more work, though. But the up side is if the data format changes in the future, you just would need to change code related to that intermediate type, and the rest would keep working.
Alternatively your could look at the attributes of serde, rusts equivalent parsing framework. You probably can do what you need with that, but I haven't delved any deeper than renaming variables to camel case.
#[derive(Clone, Debug, PartialEq, PartialOrd)]
struct Temp(f32);
impl Eq for Temp{}
impl Ord for Temp{
fn cmp(&self, other: &Self) -> std::cmp::Ordering {
self.0.total_cmp(&other.0)
// self.0.partial_cmp(&other.0).unwrap()
}
}
#[derive(Clone, Debug, PartialEq, PartialOrd)]
struct Pres(f32);
impl Eq for Pres{}
impl Ord for Pres{
fn cmp(&self, other: &Self) -> std::cmp::Ordering {
self.0.total_cmp(&other.0)
// self.0.partial_cmp(&other.0).unwrap()
}
}
#[derive(Clone, Debug)]
struct Data(u32, Temp, Pres); // time, temp, press
impl Data {
fn get_temprature(&self) -> &Temp {
&self.1
}
}
fn main() {
let v: Vec<Data> = vec![
Data(10, Temp(75.0), Pres(10000.0)),
Data(11, Temp(100.0), Pres(12000.0)),
Data(12, Temp(25.0), Pres(8000.0)),
Data(13, Temp(50.0), Pres(9000.0)),
];
let tn = v.iter().min_by_key(|d| &d.1).unwrap();
// let tn = v.iter().min_by_key(Data::get_temprature).unwrap();
println!("min: {:?}", tn.get_temprature());
let tx = v.iter().max_by_key(|d| &d.1).unwrap();
println!("max: {:?}", tx.get_temprature());
}
After a bit of hacking I've tried to use the newtypes pattern and I've replaced my own custom min method by min_by_key(). I think there is a bit of duplication with the cmp function going on I'm sure could be removed. I also can't figure out how to get
let tn = v.iter().min_by_key(Data::get_temprature).unwrap();
to compile:
error[E0631]: type mismatch in function arguments
--> src/main.rs:41:34
|
27 | fn get_temprature(&self) -> &Temp {
| --------------------------------- found signature defined here
...
41 | let tn = v.iter().min_by_key(Data::get_temprature).unwrap();
| ---------- ^^^^^^^^^^^^^^^^^^^^ expected due to this
| |
| required by a bound introduced by this call
|
= note: expected function signature `for<'a> fn(&'a &Data) -> _`
found function signature `for<'a> fn(&'a Data) -> _`
note: required by a bound in `std::iter::Iterator::min_by_key`
Your closure or an analogous one is the way to do it.
let tn = v.iter().min_by_key(|d| d.get_temprature()).unwrap();
As for why:
min_by_key is designed to work on iterators over a generic T, which might not be a reference (or Copy or even Clone), so the closure argument takes a &T.
In your case, T = &Data and so the argument must take a &&Data. But get_temprature takes a &Data. So the explicit closure is there to pierce one later of indirection.
The alternative is to clone your data, but don't do that; just use the explicit closure. It's a common pattern.
Thank you. A little later I realised the closure did not use self and that was the problem. But when I tried to change my method to the signature suggested by the compiler I still had problems. A bit of trial and error later I got this to compile:
impl Data {
fn get_temprature<'a>(d: &&'a Data) -> &'a Temp {
&d.1
}
}
...
// let tn = v.iter().min_by_key(|d| &d.1).unwrap();
let tn = v.iter().min_by_key(Data::get_temprature).unwrap();
println!("min: {:?}", Data::get_temprature(&tn));
But I was thinking I have no idea what I've done. Thanks for the explanation. I think I shall stick with the closure. Less code!
You changed things so that get_temprature takes a &&Data (and is an associated function, not a method that you can call with data.method()). A better signature would be
fn get_temprature(self: &&Self) -> &Temp
(as this is still a method).
However both of those are unidiomatic. It's nice and arguably cleaner when you can use Data::get_temprature, but when you can't, just go with the closure instead of making your signatures weird; anyone with a bit of experience will understand what's going on.
For example this won't work with either signature; without self, it's no longer a method, and even with self: &&Self, you only get one layer of "auto-ref" (and you would need two).
// yay! [compiles]
let tn = v.iter().min_by_key(Data::get_temprature).unwrap();
println!("min: {:?}", tn.get_temprature());
let data = Data(0, Temp(0.0), Pres(0.0));
// boo [does not compile] :-(
data.get_temprature();
Thanks. It is nice to know how things work under the covers. I've decided to drop the get_.... thing since I'm already using the . syntax for tuples and I now have wrapper types around my f32s.
let tn = v.iter().min_by_key(|d| &d.1).unwrap();
println!("min: {:?}", tn.1);
let tx = v.iter().max_by_key(|d| &d.1).unwrap();
println!("max: {:?}", tx.1);