Further refactors #33
Conversation
this slightly changes early calc periods (off by 1) and technically might cause double calculation when `length == threshold` but it saves 2 comparisons every lopp that is only relevant first and last case.
src/md.rs
Outdated
| flags, | ||
| unicode_range: range, | ||
| } | ||
| detectors.iter().map(|x| x.ratio()).sum() |
There was a problem hiding this comment.
why it is here instead of mean_mess_ratio?
There was a problem hiding this comment.
I was trying to redo calculations in case of index ==0 (which I skip now)
i realize it is wrong because it has to be recalculated before the trace as well.
7e085a5 fixes it but ensures double calc most of the time.
There are multiple ways to reduce this double calc including
using mean_mess_ratio: Option<f32> or calling a early return function instead of break in the if.
I've tried both and they work, but introduces a bit of complexity so i am keeping it simple for now.
There was a problem hiding this comment.
Maybe we could use condition if log_enabled!(Level::Trace) , see https://docs.rs/env_logger/latest/env_logger/ ?
There was a problem hiding this comment.
Since there are other places where the trace exists, maybe addressing that could be part of another pr?
There was a problem hiding this comment.
I don't think so. It is only one place when we have additional calculation because of logs. You could use this if to avoid double calculation of mean_mess_ratio.
There was a problem hiding this comment.
d442d08
removes recalc.
I'll look to integrate if log_enabled!(Level::Trace)
I am restricting visibility as much as possible right now.
(Minimal
puband when necessarypub(crate),pub(super))This would allow for cleaner implementations, outside api and hopefully allow more compiler optimizations.