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

Feature: new Quartiles from precalculated values #176

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions src/data/quartiles.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,33 @@ impl Quartiles {
}
}

/// Create a new quartiles struct with pre-calculated values.
///
/// - `s`: The array of the original values
/// - **returns** The newly created quartiles
///
/// ```rust
/// use plotters::prelude::*;
///
/// let quartiles = Quartiles::new_from_values(&[2, 24, 31, 39, 45]);
/// assert_eq!(quartiles.values(), [2.0, 24.0, 31.0, 39.0, 45.0]);
/// ```
pub fn new_from_values<T: Into<f64> + Copy + PartialOrd>(s: &[T; 5]) -> Self {
let s = s.to_owned();
assert!(s[0] <= s[1]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, we don't want to panic even if things are going wrong, since we are a library, we want to avoid this kind of panic.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could either:

  1. collect the array into a Vec then sort
  2. change the return type to Result<Self, SomeErrorType>
  3. leave it

Personally I would choose 3 (perhaps with an assertion string to explain what happened). If I calculate or receive quartile values and they are not in order, I want to know very directly that something is wrong.

1 is my next choice, but it could silently smooth over some logic error the user made earlier.

2 has both the previous benefits, but a big fallback since users now have to handle an error variant for what should be a very simple operation.

What do you think?

Copy link
Member

@38 38 Sep 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should panic here, since it doesn't deserve that - more importantly, panicking in a library is more like an "unrecoverable error". This is definitely not "an unrecoverable error". In today's Plotters, there are places may panic unexpectedly, but we are constantly working to get rid of them.

In principle, we don't allow any panic unless (1) there's no way handle correctly or (2) we believe panicking is impossible.

I am fine with both 1 and 2, but I am personally prefer 1. Returning result is still an overkill (And panicking in lib is even worse than that). Also if you already copied the data, sorting a 5 elements slice is very lightweight - I believe Rust's sort algorithm has optimizations for small slices.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok sounds good, I'll implement option 1 then!

assert!(s[1] <= s[2]);
assert!(s[2] <= s[3]);
assert!(s[3] <= s[4]);

Self {
lower_fence: s[0].into(),
lower: s[1].into(),
median: s[2].into(),
upper: s[3].into(),
upper_fence: s[4].into(),
}
}

/// Get the quartiles values.
///
/// - **returns** The array [lower fence, lower quartile, median, upper quartile, upper fence]
Expand Down
20 changes: 19 additions & 1 deletion src/drawing/backend_impl/piston.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,14 @@ fn make_point_pair(a: BackendCoord, b: BackendCoord, scale: f64) -> [f64; 4] {
]
}

fn make_circle(center: BackendCoord, radius: u32, scale: f64) -> [f64; 4] {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me this isn't related to the PR, right?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think @pelekhay added this commit

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So would you mind rebase to the master branch - since you are actually based on the 0.2 branch

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah my bad, I will rebase

circle(
center.0 as f64 * scale,
center.1 as f64 * scale,
radius as f64 * scale,
)
}

impl<'a, 'b> PistonBackend<'a, 'b> {
pub fn new(size: (u32, u32), scale: f64, context: Context, graphics: &'b mut G2d<'a>) -> Self {
Self {
Expand Down Expand Up @@ -150,7 +158,7 @@ impl<'a, 'b> DrawingBackend for PistonBackend<'a, 'b> {
style: &S,
fill: bool,
) -> Result<(), DrawingErrorKind<Self::ErrorType>> {
let rect = circle(center.0 as f64, center.1 as f64, radius as f64);
let rect = make_circle(center, radius, self.scale);
if fill {
ellipse(
make_piston_rgba(&style.as_color()),
Expand Down Expand Up @@ -204,3 +212,13 @@ pub fn draw_piston_window<F: FnOnce(PistonBackend) -> Result<(), Box<dyn std::er
}
None
}

#[cfg(test)]
mod test {
use super::*;
#[test]
fn test_make_circle() {
assert_eq!(make_circle((1, 1), 0, 1.0), [1.0, 1.0, 0.0, 0.0]);
assert_eq!(make_circle((1, 2), 3, 4.0), [-8.0, -4.0, 24.0, 24.0]);
}
}