-
Notifications
You must be signed in to change notification settings - Fork 8
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
Add test for Median
trait
#267
Conversation
|
||
#[test] | ||
fn test_median() { | ||
let mut empty: Vec<u32> = Vec::new(); |
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.
Do these fixtures need to be mutable?
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, just found the median trait and see that it needs to sort.
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.
Yeah, for now there is a sort, but it potentially makes the trait harder to reason about. In a follow-up it might be worthwhile to change this to a copy. Especially if it squashes the testnet4 bug
Just spit balling, but I wonder if its worth considering a trait bound implementation (got this set of traits from Claude) to DRY-ify the impl. use std::ops::Add;
use std::cmp::Ord;
pub trait Median {
type Output;
fn median(&mut self) -> Self::Output;
}
impl<T> Median for Vec<T>
where
T: Copy + Ord + Add<Output = T> + From<u8>,
{
type Output = T;
fn median(&mut self) -> T {
if self.is_empty() {
return T::from(0);
}
self.sort();
let len = self.len();
let mid = len / 2;
if len % 2 == 1 {
self[mid]
} else {
(self[mid - 1] + self[mid]) / T::from(2)
}
}
} Also, I believe there are some potential overflow cases? Haven't actually put the thought in though if the domain cares. |
Skeptical of the edit: Nevermind on |
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.
ACK 037e262
Makes sense, sparked a few follow up ideas for later.
The median time past check is failing over Testnet4 (issue #265). Here I am adding a check to be sure the
Median
trait defined in the prelude works as expectedcc @nyonson