Conversation
| if self.count_at(other_value).unwrap() < other_count { | ||
| // TODO Perhaps we should saturating sub here? Or expose some form of | ||
| // pluggability so users could choose to error or saturate? Both seem useful. | ||
| // It's also sort of inconsistent with overflow, which now saturates. |
There was a problem hiding this comment.
Hmm, yes, it's a good question..
One thing to do would be to have a variant in SubtractionError (and possibly also AdditionError) called PartialResult (or something like that), which contains the Histogram when using saturating subtract/add. That way the user can either chose to deal with it as an error, or they can match that case and ignore the error.
| // TODO consider split out addition and subtraction cases; this isn't gaining much by | ||
| // unifying since we have to test all the cases anyway, and the TODO below is marking a case | ||
| // that might well be impossible but seems needed because of the (possibly false) symmetry | ||
| // with addition |
| // count to exceed subtrahend count for a given value, we shouldn't ever need | ||
| // to resize to subtract. | ||
| // Anyway, at the very least, we know it shouldn't underflow. | ||
| *c = (*c).checked_sub(&count).expect("count underflow after resize"); |
There was a problem hiding this comment.
Maybe make this unreachable!() then?
There was a problem hiding this comment.
It would take more subtraction testing to make me feel confident that this is in fact unreachable, so I'd prefer to wait for #36 for that change
| /// | ||
| /// Note that the return value is capped at `u64::max_value()`. | ||
| pub fn median_equivalent(&self, value: u64) -> u64 { | ||
| // TODO isn't this just saturating? |
There was a problem hiding this comment.
Yup. I guess I just didn't know about saturating_add at the time. Happy to have it changed.
There was a problem hiding this comment.
Actually, it looks like this should never overflow, so I'll use checked_add (and a different PR, just in case that discussion might take us in a different direction from this PR)
| if self.min_nz() != other.min_nz() { | ||
| return false; | ||
| } | ||
| // TODO may panic? Does the above guarantee that the other array is at least as long? |
There was a problem hiding this comment.
Ah, good point. Yes, this has a bug.
|
Looks good. I'll leave it open to hear your thoughts on the inline comments above, but happy to merge after that. |
I'd intended to do this later but it got in the way of benchmarks for deserialization to panic whenever things overflowed.
There are more overflows to handle (TODOs added) but they aren't inhibiting me so I left them for later. Each one needs a benchmark and tests, so they take a little time.
Fortunately, this change has no effect whatsoever on performance, at least as far as I could measure. This is a somewhat tricky thing to benchmark because the thing measured is significantly faster than, say, random number generation, so I did the best I could by precalculating numbers to record (or histograms to add/subtract) and then simply iterating across that vec in the hot loop.