Implement CubeHelix sampling function #31

safareli · 2017-08-26T17:00:42Z

This fixes cubehelix sampling issue

safareli

I'm not particularly happy with name ColorScaleBuilder, we might move it with corresponding functions in separate module too.

safareli · 2017-08-26T17:01:04Z

src/Color.purs

+radians = pi / 180.0
+
+cubehelixMix :: Number -> Mixer -- TODO fix alpha
+cubehelixMix gama (HSLA ah' as' al' _) (HSLA bh' bs' bl' _) =


ported from https://github.com/d3/d3-plugins/blob/master/cubehelix/cubehelix.js#L13

Let's maybe add this as a comment in the code?

Also, should be gamma instead of gama.

safareli · 2017-08-26T17:01:35Z

src/Color.purs

+    al = al'
+    bl = bl' - al
+
+  -- NOTE not sure why isNan check is needed so I have not ported it


I will remove this comments, as still not sure why it's needed 🗡

Yes, let's remove the NaN checks. I think it's only needed if the Color could possibly have NaN values internally..

safareli · 2017-08-26T17:02:32Z

src/Color.purs

+  -- NOTE not sure why isNan check is needed so I have not ported it
+  -- if (isNaN(bs)) bs = 0, as = isNaN(as) ? b.s : as;
+  -- if (isNaN(bh)) bh = 0, ah = isNaN(ah) ? b.h : ah;
+  in \t ->


I think it should be much better in performance, what's your thoughts?

Possibly. Good idea.

safareli · 2017-08-26T17:08:31Z

src/Color/Scale.purs

@@ -53,19 +64,26 @@ stopColor :: ColorStop -> Color
 stopColor (ColorStop c _) = c

 -- | A color scale.
-data ColorScale = ColorScale ColorSpace Color (List ColorStop) Color
+data ColorScaleBuilder = ColorScaleBuilder Color (List ColorStop) Color


Exporting this is very useful so users could write their own helper functions, without waiting for their PR to be merged. Before I needed to define my own type for it see this

Also if we have separate type which does not contain ColorSpace then we can implement combineScale :: Number → Scale → Scale → Scale which i did in the link above. If the Scale contained colorSpace then we would have to discard one of them, which I think will not be a good thing to do.

What about just naming this ColorStops or ColorStopList?

safareli · 2017-08-26T17:11:30Z

src/Color/Scale.purs

-uniformScale mode b middle e = colorScale mode b stops e
+uniformScale mode b middle e = ColorScale mode $ uniformScale' b middle e
+
+uniformScale' :: forall f. Foldable f => Color -> f Color -> Color -> ColorScaleBuilder


this ' functions might be useful when someone is working on ColorScaleBuilder and might not care about ColorSpace. Like I did here

Okay. Can we please add a docstring?

safareli · 2017-08-27T09:14:56Z

Stack overflows on HSV -> HSL -> HSV, HSL -> HSV -> HSL test :/

sharkdp · 2017-08-27T09:23:38Z

Thank you for working on this. Let me know when it's ready to review.

safareli · 2017-08-27T09:28:45Z

It's ready for review

sharkdp

Thank you for working on this! There's a few things I'd like to discuss first, before merging this - please see the inline-comments.

sharkdp · 2017-08-27T09:50:18Z

src/Color.purs

-  where h' = if h == 360.0 then h else h `modPos` 360.0
-        s' = clamp 0.0 1.0 s
+hsla h s l a = HSLA h s' l' a'
+  where s' = clamp 0.0 1.0 s


I don't really like that we have to change this:

It forces us to insert clipHSLHue calls in multiple places (see below)

Worse, it forces us to remember that we have to call clipHSLHue when using hue values (in the future)

Why is this necessary?

it's needed for coubehelix mix of colors.
for example if we want gradient from volor eith hue 350 to color with hue 370.
without this change we will get gradient from 350 down to 10 where wi will have length of 340 in hue, not up to 370 where length will be 20. and the distinction between 370 and 10 might be useful for other sampling functions too

to not forget cliping we could use toHSLA and do not patern match on constructor, or sormthing else.

you can revert this part and see that coubhelix gradient will look like

I see, okay.

I have just pushed a refacto of this issue.
let me know if you like the approach, or if you can think of different approach to issue?

In this case, I rather prefer the previous approach, to be honest. With both hue and hueOriginal as fields, we have to keep track of always keeping both in sync. Sorry 😄

Thinking about this a bit more, I guess it's okay to allow for angles outside of [0°, 360°].

I've reverted most of the changes but made this change, so we are not forced to remember clipping Hue:

data Color = HSLA Hue Number Number Number newtype Hue = UnclippedHue Number clipHue :: Hue -> Number clipHue (UnclippedHue x) = x `modPos` 360.0

sharkdp · 2017-08-27T09:53:24Z

src/Color.purs

@@ -500,10 +505,13 @@ interpolateAngle fraction a b = interpolate fraction shortest.from shortest.to
    dist { from, to } = abs (to - from)
    shortest = unsafePartial (fromJust (minimumBy (comparing dist) paths))

+
+type Mixer = Color -> Color -> Number -> Color


I'm fine with the type synonym, but I'd like to add a short doc-string (.. a function that interpolates between two colors..).

sharkdp · 2017-08-27T09:54:00Z

src/Color.purs

@@ -538,6 +546,32 @@ mix Lab c1 c2 frac = lab
    f = toLab c1
    t = toLab c2

+radians :: Radians


I would move this definition inside cubehelixMix, if it is not needed anywhere else.

sharkdp · 2017-08-27T09:54:43Z

src/Color.purs

+radians = pi / 180.0
+
+cubehelixMix :: Number -> Mixer -- TODO fix alpha
+cubehelixMix gama (HSLA ah' as' al' _) (HSLA bh' bs' bl' _) =


Let's maybe add this as a comment in the code?

sharkdp · 2017-08-27T09:55:11Z

src/Color.purs

+radians :: Radians
+radians = pi / 180.0
+
+cubehelixMix :: Number -> Mixer -- TODO fix alpha


Minor comment: I'd prefer mixCubehelix.

sharkdp · 2017-08-27T10:04:53Z

src/Color/Scale.purs

-uniformScale mode b middle e = colorScale mode b stops e
+uniformScale mode b middle e = ColorScale mode $ uniformScale' b middle e
+
+uniformScale' :: forall f. Foldable f => Color -> f Color -> Color -> ColorScaleBuilder


Okay. Can we please add a docstring?

sharkdp · 2017-08-27T10:12:25Z

src/Color/Scale.purs

+sample :: Sampler ColorScale
+sample (ColorScale mode scale) = mkSimpleSampler (mix mode) scale
+
+type Sampler a = a -> Number -> Color


IMHO: this type synonym just adds mental overhead when reading the type definitions below.

sharkdp · 2017-08-27T10:13:13Z

src/Color/Scale.purs

 -- | Get the color at a specific point on the color scale (number between 0 and
 -- | 1). If the number is smaller than 0, the color at 0 is returned. If the
 -- | number is larger than 1, the color at 1 is returned.
-sample :: ColorScale -> Number -> Color
-sample scale@(ColorScale mode b middle e) x
+mkSimpleSampler mixer (ColorScaleBuilder b middle e) x


Why is there a docstring between type declaration and function implementation?

sharkdp · 2017-08-27T10:14:39Z

src/Color/Scale.purs

+colors scale = colors' (sample scale)
+
+-- | Takes sampling function and returns A list of colors that is sampled from
+-- | a color scale. The number of colors can be specified.


Does colors' have to be a top-level function?

sharkdp · 2017-08-27T10:15:34Z

src/Color/Scale.purs


-cssColorStops scale@(ColorScale _ b middle e) = cssColorStops csRGB
+cssColorStopsSample :: Sampler ColorScaleBuilder -> Int -> ColorScaleBuilder -> String


What are these for? Can you please add some comments/documentation?

safareli · 2017-08-27T21:21:43Z

src/Color.purs

 mix HSL c1 c2 frac = hsla
-    (interpolateAngle frac f.h t.h)
+    (interpolateAngle frac f.h t.h) -- NOTE here we might benefit from using uncliped Hue?


Do you see use of UnclippedHue here?

Yes, good idea!

safareli · 2017-08-27T21:32:48Z

src/Color.purs

+
+-- | Mix two colors by [Dave Green's `cubehelix'](http://www.mrao.cam.ac.uk/~dag/CUBEHELIX/) interpolation between them.
+-- | Takes gamma correction value as an argument and retunrs Interpolator.
+-- | For more details see: [d3-plugins/cubehelix](https://github.com/d3/d3-plugins/tree/40f8b3b91e67719f58408732d7ddae94cafa559a/cubehelix#interpolateCubehelix)


do links like this work in pursuit?

I think so.

safareli · 2017-08-27T22:09:57Z

I have added comments to new functions, let me know if you spot some typos or would like different wording.

sharkdp · 2017-08-28T05:51:48Z

I don't have any time today but I'll try to review it in full tomorrow. Thank you very much.

Do you know why the tests started failing now? Why did the stackoverflow only appear now?

safareli · 2017-08-28T09:41:44Z

Do you know why the tests started failing now? Why did the stackoverflow only appear now?

Not sure, when I was adding the test which is now failing it was raising stackoverflow at first and because of that I used smaller numbers.

safareli · 2017-08-29T13:14:40Z

I just found out that it's allowed to have hue be out of range [0,360]

https://www.w3.org/TR/css3-color/#hsl-color

HSL colors are encoding as a triple (hue, saturation, lightness). Hue is represented as an angle of the color circle (i.e. the rainbow represented in a circle). This angle is so typically measured in degrees that the unit is implicit in CSS; syntactically, only a is given. By definition red=0=360, and the other colors are spread around the circle, so green=120, blue=240, etc. As an angle, it implicitly wraps around such that -120=240 and 480=120. One way an implementation could normalize such an angle x to the range [0,360) (i.e. zero degrees, inclusive, to 360 degrees, exclusive) is to compute (((x mod 360) + 360) mod 360). Saturation and lightness are represented as percentages. 100% is full saturation, and 0% is a shade of gray. 0% lightness is black, 100% lightness is white, and 50% lightness is “normal”.

This makes me think that we should remove clipping from cssStringHSLA. what you think?

Besides cssStringHSLA we call clip in to{RGB,HSL,HSV}A.
Also There is no way for user to get unclipped Hue. I think we should add toHSLA' or similar, which returns { h :: Hue, s :: Number, l :: Number, a :: Number } and export Hue + clipHue. thoughts?

safareli · 2017-08-29T14:22:03Z

I came to d3/d3-color here cubehelix is implemented as color space, not as just an interpolation.

If we try same thing I see it as creating Color.cobehelix which takes h s l a which converts CobeHelix color to HSLA to construct Color object. Then adding CobeHelix to Color.ColorSpace and we will add case for this colorspace in Color.mix, where Colors a b will be converted to CobeHelix mixed and then back to HSLA.

But I don't quite see how to implement, will need to dig into implementation
Will I do not get is what's the range of saturation, so I think I will stop on this until i hear back on d3/d3-color#34

here is a:

A two-dimensional cross section of the Cubehelix color space: from top to bottom, hue ranges from 0° to 360°; from left to right, lightness ranges from 1 to 0. Saturation is a constant 1.
Raw

if you try to look at that image with grayscale enabled with on our PC/mobile you will see that lightness is monotonic.

This is Color picker in CobeHelix space.

sharkdp · 2017-08-29T19:54:46Z

Not sure, when I was adding the test which is now failing it was raising stackoverflow at first and because of that I used smaller numbers.

Thanks for fixing those

This makes me think that we should remove clipping from cssStringHSLA. what you think?

Yeah, good point. Thanks for the reference.

Besides cssStringHSLA we call clip in to{RGB,HSL,HSV}A.
Also There is no way for user to get unclipped Hue. I think we should add toHSLA' or similar, which returns { h :: Hue, s :: Number, l :: Number, a :: Number } and export Hue + clipHue. thoughts?

Maybe it's actually best if we return unclipped values everywhere. Do we need the clipped values at some point? 😄

But I don't quite see how to implement, will need to dig into implementation
Will I do not get is what's the range of saturation, so I think I will stop on this until i hear back on d3/d3-color#34

Cool. Thanks for looking into this. Sounds like a good option to me (adding it as a ColorSpace).

safareli · 2017-08-30T12:15:05Z

Do we need the clipped values at some point?

We need to clip for toRGBA, on the other side for some applications clipped hue might be needed. So what you are suggesting is to just store unclipped hue as Number and expose clipHue :: Number -> Number which will be used only in toRGBAand in client code ? or what

About ColorSpace I don't yet understand how we can do that I have some open questions in the issue will take a look into it in future maybe.

So to merge this we just need to decide what should we don on clipping.

as of spec it's fine https://www.w3.org/TR/css3-color/#hsl-color

sharkdp · 2017-08-31T17:00:11Z

We need to clip for toRGBA, on the other side for some applications clipped hue might be needed. So what you are suggesting is to just store unclipped hue as Number and expose clipHue :: Number -> Number which will be used only in toRGBAand in client code ? or what

I think it's perfectly fine the way you did it right now, thank you very much.

sharkdp · 2017-08-31T17:57:07Z

@safareli

I have added cubehelix :: ColorScale. Thought you might find this useful.

I've also added the mixCubehelix function and the cubehelix colorscale to the interactive documentation: http://sharkdp.github.io/purescript-colors/

safareli · 2017-08-31T20:17:27Z

nice!

safareli · 2017-08-31T20:18:37Z

Let me know when you release new version :P

sharkdp · 2017-08-31T20:52:14Z

Let me know when you release new version :P

Done. Thanks again!

sharkdp · 2017-08-31T21:37:06Z

I've also added a small interactive cubehelix experiment at the bottom of the page:
http://sharkdp.github.io/purescript-colors/

(similar to https://www.mrao.cam.ac.uk/~dag/CUBEHELIX/cubetry.html)

safareli · 2017-08-31T21:46:21Z

sweet 🍓

safareli added 7 commits August 25, 2017 23:23

add ColorScaleBuilder

a125216

add cubelix sampling function

62c3d99

do not clip Hue before it's needed

d0f9f90

This fixes cubehelix sampling issue

resotre colors behaviour

2f832ba

restore some colorscale spaces

a4fc99e

add modify'

7172cb5

fix colors comment

eb0c33a

safareli commented Aug 26, 2017

View reviewed changes

sharkdp requested changes Aug 27, 2017

View reviewed changes

safareli added 3 commits August 27, 2017 22:56

update Color to be record

5fe1b09

Color type refactor (Hue = UnclippedHue Number)

76da661

review iteration changes

e32c86d

safareli commented Aug 27, 2017

View reviewed changes

safareli force-pushed the cube branch from c3bff21 to 0f42602 Compare August 27, 2017 21:30

add some comments

9410e5c

safareli force-pushed the cube branch from 0f42602 to 9410e5c Compare August 27, 2017 21:31

safareli commented Aug 27, 2017

View reviewed changes

update comments

0b90a09

safareli changed the title ~~implement CubeHelix staff~~ Implement CubeHelix sampling function Aug 29, 2017

safareli added 2 commits August 29, 2017 17:00

decrees numbers in tests to not fire stack owerflow

4e7b6ac

restore alowed 360 value for hue

c28d5fd

safareli mentioned this pull request Aug 29, 2017

What’s the range of *s* in Cubehelix? d3/d3-color#34

Closed

Do not clip Hue in cssStringHSLA

4a870db

as of spec it's fine https://www.w3.org/TR/css3-color/#hsl-color

sharkdp approved these changes Aug 31, 2017

View reviewed changes

sharkdp merged commit bcf09c0 into purescript-contrib:master Aug 31, 2017

safareli mentioned this pull request Sep 1, 2017

update ps-colors version range purescript-contrib/purescript-css#72

Merged

safareli deleted the cube branch September 1, 2017 13:24


		cssColorStops scale@(ColorScale _ b middle e) = cssColorStops csRGB
		cssColorStopsSample :: Sampler ColorScaleBuilder -> Int -> ColorScaleBuilder -> String

Implement CubeHelix sampling function #31

Implement CubeHelix sampling function #31

Uh oh!

Conversation

safareli commented Aug 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

safareli left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

safareli commented Aug 27, 2017

Uh oh!

sharkdp commented Aug 27, 2017

Uh oh!

safareli commented Aug 27, 2017

Uh oh!

sharkdp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

safareli Aug 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

safareli commented Aug 26, 2017 •

edited

Loading

safareli Aug 27, 2017 •

edited

Loading