-
-
Notifications
You must be signed in to change notification settings - Fork 372
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
valToIdx: PEBKAC or bug? #311
Comments
I believe this function would need to do something different in the case where https://github.com/leeoniya/uPlot/blob/master/src/uPlot.js#L150 |
yes, there may very well be a bug/oversight here since the internal array for there are a bunch of places in the code that check for distr:2 and swap which array must be used (as you've discovered). i'm not terribly happy with the whole thing since it leads to other oddities like #212. the fix should be simple though by branching in additional places, as you point out. |
i'm on holiday currently and not near a computer, so it'll be a few days before i can verify and fix or review any PRs. the correct fix for this might be to adjust https://github.com/leeoniya/uPlot/blob/master/src/uPlot.js#L1611 with |
Enjoy your vacation, thanks for verifying! You could eventually want to refactor the whole distr thing once it gets too unweildy so for now I think it's probably fine to make it a little more unweildy with the extra branching. ;-) |
i think this is fixed by the referenced commit (along with a few related api methods!). let me know how this works. thanks! code i used to test: <!doctype html>
<html>
<head>
<meta charset="utf-8">
<title>u.valToIdx()</title>
<meta name="viewport" content="width=device-width, initial-scale=1">
<link rel="stylesheet" href="../dist/uPlot.min.css">
</head>
<body>
<script src="../dist/uPlot.iife.js"></script>
<script>
let xs = [1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30];
let vals = [-10,-9,-8,-7,-6,-5,-4,-3,-2,-1,0,1,2,3,4,5,6,7,8,9,10];
let data = [
xs.map((t, i) => t * 3),
xs.map((t, i) => vals[Math.floor(Math.random() * vals.length)]),
xs.map((t, i) => vals[Math.floor(Math.random() * vals.length)]),
xs.map((t, i) => vals[Math.floor(Math.random() * vals.length)]),
];
const opts = {
width: 1920,
height: 600,
title: "u.valToIdx()",
scales: {
x: {
time: false,
distr: 2,
},
},
series: [
{},
{
stroke: "red",
fill: "rgba(255,0,0,0.1)",
},
{
stroke: "green",
fill: "rgba(0,255,0,0.1)",
},
{
stroke: "blue",
fill: "rgba(0,0,255,0.1)",
},
],
};
let u = new uPlot(opts, data, document.body);
data[0].forEach((v, i) => {
console.log({
val: v,
realIdx: i,
valtoIdx: u.valToIdx(v)
});
});
</script>
</body>
</html> |
wait, this caused some unintended side-effects |
looks like i'll need to bite the bullet and fix this holistically together with #212. otherwise it'll become an even more confusing mess. |
Sounds like a normal vacation ;-) Do you still want me to test the latest? Also, why is |
after some more pondering, both this issue and #212 stem from the way that uPlot masks the internals of how the thing to understand for if you were to do this in userland, it would be more verbose, but more obvious. const labels = [36,250,251,900];
const data = [
labels.map((v, i) => i), // x values (ordinal positions, spaced linearly)
[87,29,75,13], // y values
];
const opts = {
axes: [
{
incrs: [1,2,5,10,20,50,100,200,500,1000...],
values: (u, splits) => splits.map(i => labels[i]),
},
],
series: [
{
value: (u, i) => labels[i]
},
],
}; (plus a bit more)...but that's really all that Lines 15 to 35 in e796aa6
long story short, i think it's working as designed if you deal/think in x-indices-as-values whenever in |
I had assumed that So I completely agree with you. I might even suggest that Thanks! |
as tempting as that sounds, i think it's super ugly to have distr:2's labels outside of opts or data and not have it tightly integrated with i really would like to figure out a concise way to abstract the implementation details of distr:2 and make it behave according to expectations; there would be too much convenience lost by simply dropping support, imo. |
It seems then that you've got the starts for a more general abstraction
above. One that can apply to multiple values of distr. Data -> labels and
indexes.
The convenience is fine and appreciated but it seems to me that it is not
intrinsic to the library and thinking about it as separate might help. So
perhaps when opts encounter a specific distr value, then a specific
transformation is executed and from there on distr is meaningless
internally, similar to what you've done above.
…On Sat., Sep. 26, 2020, 8:33 a.m. Leon Sorokin, ***@***.***> wrote:
I might even suggest that distr:2 be removed or become unsupported
sometime soon.
as tempting as that sounds, i think it's super ugly to have distr:2's
labels outside of opts and not have it tightly integrated with .setData(),
etc.
i really would like to figure out a concise way to abstract the
implementation details of distr:2 and make it behave according to
expectations; there would be too much convenience lost by simply dropping
support, imo.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#311 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AJDE4VGCKIPSYEWIYJDJMVDSHXNQJANCNFSM4RX2JVCA>
.
|
i've made a couple incomplete attempts at concealing the internals but basically ended up in a similar place to #77: it's a lot of extra internal bookkeeping for not a lot of added value. i could not come up with a generic userland wrapper that can gracefully integrate for the time being, since the explanation is pretty straightforward [1] and the boilerplate reduction is significant, i think the current i'm marking this as a duplicate of #212, since it's essentially the same root issue. [1] "API methods involving ordinal |
This may betray a total misunderstanding of the API but, shouldn't the following always return 0 even if
distr = 2
?u.valToIdx(u.data[0][0])
For me, it always seems to return the right-most index (as u.data[0][0] is a datetime and is usually going to be larger than the highest index). I believe there is an interaction with
distr = 2
that is causing this, but I'm not sure how to inverse it.https://github.com/leeoniya/uPlot/blob/master/dist/uPlot.d.ts#L97
Any suggestions on how can I otherwise find the nearest x index through the API/
Thanks.
The text was updated successfully, but these errors were encountered: