time_per_step() should return f64, not Duration. about indicatif HOT 6 CLOSED

I doubt this was a problem with Duration. After looking at the code it uses f64 in the buf vector instead of Durations. Duration supports addition and there is no need to convert to f64 before summing it up.

Floats are imprecise by construction. The time API works on nanoseconds internally and so should we. conversion to f64 should be the last step.

from indicatif.

Perhaps I've made a mistake by suggesting a solution in the issue's title instead of just describing it.

After looking at the code it uses f64 in the buf vector instead of Durations. Duration supports addition and there is no need to convert to f64 before summing it up.

I think storing actual durations as Duration would be perfectly fine. It's different here though — what we store here is not a duration, but rather speed^−1 — duration divided by number of steps. So the loss of precision happens anyway, in the moment of division itself and by rounding the time_per_step to nanoseconds. And exactly this rounding to nanoseconds makes the indicatif unusable with transfer rates of the order of GB/s.

By the way, f64 is able to store a number of nanoseconds losslessly up to about 7 months (and µs to about 500 years), so I wouldn't worry too much about the loss of precision.

from indicatif.

I mean why store the result of a lossy calculation instead of both values for each step. Then sum the components separately and divide afterwards.

Although I'm wondering why its not just using 0.9*last + 0.1*current for estimating instead of this buffer construction.

from indicatif.

Encountered this issue also. The nanosecond precision used by Duration is really not enough when dealing with time per step calculations. Rates of >1e9/s can be relatively easy to achieve when dealing with things like reading bytes from modern SSDs. Using f64 specifically to compute Estimate::time_per_step() is quite reasonable, as it is not dealing with precise durations.

from indicatif.

I'm happy to review a PR for this.

from indicatif.

Incoming!

from indicatif.