graph, store: Make sure vid batching works with large vids#5970
graph, store: Make sure vid batching works with large vids#5970
Conversation
zorancv
left a comment
zorancv
left a comment
There was a problem hiding this comment.
This is clear improvement over the current situation and is equivalent to it from algebraic point of view, so I approved it with one remark for consideration in the future. Nice that you added a regression test case!
| // as f64) as i64 < points[j]` | ||
| // | ||
| // We therefore try to only convert differences between points to f64 | ||
| // which are much smaller. |
There was a problem hiding this comment.
Here the assumption is that differences of the subsequent points is never more than 2'000'000 blocks, as we have only 21 bits of the mantissa available (53 of f64 - 32 of the shift)? If that's not absolutely guaranteed I would suggest to convert all the calculations to work with i128. The bins_size could be represented as a ratio of two integers. The only drawback are somewhat slow divisions...
There was a problem hiding this comment.
That's a really good point. I'll leave this as-is for now, but if I ever need to touch it, I'll adopt your suggestion.
Changing to the new vid scheme of `block_num << 32 + sequence_num` revealed some numerical problems in the batching code.
lutter
force-pushed
the
lutter/ogive-roundoff
branch
from
1a634c4 to
72834fd
Compare
2 participants
Changing to the new vid scheme of
block_num << 32 + sequence_numrevealed some numerical problems in the batching code.