herbie-fp / odyssey

A platform for exploring floating-point expressions :boat:
https://herbie-fp.github.io/odyssey/
MIT License
17 stars 0 forks source link

(Bucketing) Selected point not showing up on all error plots with multiple variables #87

Closed jaelafield closed 2 months ago

jaelafield commented 2 months ago

Currently, selecting a point on the error plot for one variable doesn't always cause a point to select on error plots for other variables in the expression:

Screenshot 2024-08-15 at 3 20 28 PM

(@elmisback let me know if I misunderstood your explanation + solution idea)

elmisback commented 2 months ago

That's roughly correct! For now you can think of the buckets as being applied like so:

Say we have 10 X values, [1, 2, 3, 4, 5, 6, 7, 8, 9, 10], but we can only show 2 values. Bucketing evenly, we have two sets: [[1, 2, 3, 4, 5], [6, 7, 8, 9, 10]]. We just take the head of each bucket for display: [1, 6]. But we'd like to maintain the whole list of buckets so we can look values inside the buckets up. This can be done by saving the lists somewhere or by regenerating the buckets when we need to look them up, since the process is deterministic.

jaelafield commented 2 months ago

In 1:1 8/27, made an implementation plan with Edward:

I've implemented a first attempt at this plan, now just debugging and trying to get it right!

jaelafield commented 2 months ago

The current bug is that const pIdx = dataPoints[expIdx].indexOf(pValue); always results in -1 for pValues of the second variable in an expression (e.g. 'y' of 'x+y').

This occurs even though the value is clearly contained in the dataPoints collection. (As it must be, since dataPoints is extracted from the same list used to plot the actual points.) Example:

Screenshot 2024-08-29 at 5 09 56 PM

It seems like it must be an equality issue, so I tried our idea of comparing in float form, rather than ordinal. Strangely though, this isn't a problem for the first variable, only the second (or 3rd, etc), regardless of float vs ordinal.

jaelafield commented 2 months ago

Resolved in 50dc5e5!

See final bug fix description here.

jaelafield commented 2 months ago

Done (one last fix was added), can be merged into main with go-ahead!