Feature: new Quartiles from precalculated values - #176
Conversation
Sorry, something went wrong.
38
left a comment
There was a problem hiding this comment.
Hi sorry for getting back to you so late, I looked at the code, it's goods good to me mostly. But I don't think we should panic at this point anyway, as long as we are a library we should avoid any unexpected inputs and returns error in most of the case.
Thanks again for the help!
Sorry, something went wrong.
| /// ``` | ||
| pub fn new_from_values<T: Into<f64> + Copy + PartialOrd>(s: &[T; 5]) -> Self { | ||
| let s = s.to_owned(); | ||
| assert!(s[0] <= s[1]); |
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
We could either:
- collect the array into a Vec then sort
- change the return type to Result<Self, SomeErrorType>
- 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?
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
Ok sounds good, I'll implement option 1 then!
Sorry, something went wrong.
| ] | ||
| } | ||
|
|
||
| fn make_circle(center: BackendCoord, radius: u32, scale: f64) -> [f64; 4] { |
There was a problem hiding this comment.
To me this isn't related to the PR, right?
Sorry, something went wrong.
There was a problem hiding this comment.
I think @pelekhay added this commit
Sorry, something went wrong.
There was a problem hiding this comment.
So would you mind rebase to the master branch - since you are actually based on the 0.2 branch
Sorry, something went wrong.
There was a problem hiding this comment.
ah my bad, I will rebase
Sorry, something went wrong.
I recently had a dataset that took hours to generate, so I wanted a workflow like:
This PR allowed me to provide pre-calculated values for the quartile range as desired
This implementation simply panics if the values aren't in a sensible order