- Notifications
You must be signed in to change notification settings - Fork301
Feature: newQuartiles from precalculated values#176
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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.
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!
| /// ``` | ||
| 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.
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.
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.
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?
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.
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.
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.
Ok sounds good, I'll implement option 1 then!
| @@ -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] { | |||
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.
To me this isn't related to the PR, right?
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.
I think@pelekhay added this commit
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.
So would you mind rebase to the master branch - since you are actually based on the 0.2 branch
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 my bad, I will rebase
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