Skip to content
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

Sorted ordinal regression #151

Closed
techniq opened this issue Oct 13, 2023 · 14 comments
Closed

Sorted ordinal regression #151

techniq opened this issue Oct 13, 2023 · 14 comments

Comments

@techniq
Copy link
Contributor

techniq commented Oct 13, 2023

8.0.0 (PR: #148) appears to have a regression when using Date instances - techniq/layerchart#54.

I haven't looked exhaustively, but I think this affects many of the ordinal examples (I could be wrong, but this might be affecting the order of the Pie.

Is it possible to:

  • opt-out of this functionality, or preferable make it opt-in?
  • better handle Date instances
@mhkeller
Copy link
Owner

Can you make a LayerCake-only repro in the REPL so I can check it out?

@techniq
Copy link
Contributor Author

techniq commented Oct 14, 2023

I just tweaked the column example... https://svelte.dev/repl/2d4d21d550c446f181b067490fcc25bb?version=3.46.2

Note, if you leave the date's as strings... they are sorted as expected...

image

but if they are Date instances, they sort oddily...

image

So far I've only looked into this Date example... although I'm a little leery of always sorting ordinal data, especially without a way to opt-out (although I don't have a good example at the moment, so maybe it's not an issue in practice).

@mhkeller
Copy link
Owner

mhkeller commented Oct 14, 2023

Thanks I’ll take a look. You can always override it by setting the xDomain manually.

xDomain = […new Set(data.map(d => d[xKey])]

@mattlangeman
Copy link

I believe the oddly sorted dates in the example above are getting sorted to the toLocalString representation of the date objects, which starts with day of the week. (Friday, Monday, Saturday, Sunday, Thursday, Tuesday, Wednesday).

https://svelte.dev/repl/b1a6573b77244700af2dd9ac4350abe1?version=4.2.1

@mhkeller
Copy link
Owner

Looks like adding a sort function should fix it https://svelte.dev/repl/28f554b370cf4cc484a5153e257888ca?version=4.2.1

@mhkeller
Copy link
Owner

To say more about whether this is a good default, my thinking was the alternative is it will display in the order it appears in your data, which has no real guarantees and may not be something people think of when trying to debug stuff. I figured a sorted domain was a sensible default but mostly exists to display something halfway decent on first load and people can always override it if they want the domain to be different.

the previous behavior also led to some unexpected behavior when using color scales in combination with data filtering. lets say you had a scatterplot or a beeswarm and you wanted to highlight a few elements and map to a color range of ['#ccc', '#f0c']. Whether the initial highlighted state was #ccc or #f0c would purely be based on whether that element appeared in the initial dataset, which is kind of weird. if you're filtering your data based on user input, the highlight color scheme would swap. granted, you could fix this by setting the domain order manually but i thought that this kind of random domain swapping behavior was a little too weird and the expectation when using a filter would be that the order of the color domain remains the same.

tl;dr choosing between a random sort order or a sorted one as an initial state that can be overridden, it seemed to make more sense to have a stable order

@mhkeller
Copy link
Owner

@techniq @mattlangeman take a look at what I have in #151, which I think should fix it for you. I'll let it sit for a few hours before merging it in case there's something I missed.

@mhkeller
Copy link
Owner

Fixed in 8.0.1

@techniq
Copy link
Contributor Author

techniq commented Oct 14, 2023

@mhkeller I just updated LayerChart to use 8.0.1 and still seeing the issue...

image

I also pulled up the LayerCake REPL I made earlier, and saw it pull 8.0.1, and it looks like it's still experiencing the issue.

image

@techniq
Copy link
Contributor Author

techniq commented Oct 15, 2023

TBH I'm not sure why #152 didn't fix this. I've been reading through the change in #148 and the tests added, and everything checks out.

@techniq
Copy link
Contributor Author

techniq commented Oct 15, 2023

@mhkeller Looks like a publishing issue.... what I see in 8.0.1 doesn't match Git... https://unpkg.com/browse/layercake@8.0.1/dist/lib/calcUniques.js

image

@mhkeller
Copy link
Owner

Sorry – still not used to the new build system with svelte kit. 8.0.2 should be properly built now...

@mhkeller
Copy link
Owner

Thanks for the catch

@techniq
Copy link
Contributor Author

techniq commented Oct 15, 2023

No worries... looks good to me now. I'll let you know if I happen to notice anything else (that I struggle to change/workaround otherwise). I really appreciate your help on this.

image

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants