fix presorted not working with color scales

[TheLostLambda]

The newly added reference test used to throw a runtime error — it now produces the expected plot.

AlgebraOfGraphics.jl/src/scales.jl

Line 88 in c2751aa

i = findfirst(isequal(u), c.keys)

shows the code that caused the failure. The Presorted("key") would not compare equal to the "key" in the color mapping.

This feels like a reasonable way for isequal(Presorted) to behave, as it's in line with the existing definition that ignores the .i field.

This is my first PR to this repo and one of my first to Julia, so let me know if I've missed anything!

[jkrumbiegel]

Hi, thanks for the PR!

So, I don't think changing isequal for Presorted is the way to go here, as this is a fundamental operation in Julia that shouldn't generally be messed with. I did this for comparison between Presorteds only as a shortcut, it might not be the cleanest solution there either.

I've thought about the actual problem you're trying to solve here before, which is that it's annoying to specify palettes with data => style pairings if the data has been wrapped. But this doesn't only apply to presorted but also other wrappers like nonnumeric. So maybe what we can do is to unwrap all known wrapper values before doing the palette lookups. This way, we don't have to change isequal behavior at all.

[TheLostLambda]

Hi @jkrumbiegel !

I think you're right that that sounds like a much cleaner overall approach! I was trying to keep what I was touching here to a minimum, but I'm happy to make that slightly larger, more "proper" fix!

It may be a little while before I have the time for it, but I'll keep you posted!

[TheLostLambda]

Perhaps there should be an abstract type for these wrappers, then all of the current wrappers can be a subtype of that?

That way users could also add their own wrappers and just "inherit" from that abstract wrapper type so that they are automatically unwrapped too?

[jkrumbiegel]

I'm usually not a fan of abstract types because they only work for one thing, while simply defining methods / traits can be used for however many behaviors.

[TheLostLambda]

A note for myself (something to include in this PR): presorted also breaks LaTeX and rich-text strings somehow (when used as layout labels).

[TheLostLambda]

Another note for myself (something that might be fixed by this proper unwrapping) is that presorted and renamer don't appear to work together currently!

EDIT: And rich() text can't be sorted, so it can't be used in row or col mappings? Maybe that's out of scope for this PR though...