Multicolors #10

jeammimi · 2017-03-03T14:01:50Z

Added the possibility that color is a string a list of string or a 2d array of string

maartenbreddels

Looking forward to this pulled in!

maartenbreddels · 2017-03-03T14:21:55Z

js/src/volume.js

@@ -310,6 +308,7 @@ var ScatterView = widgets.WidgetView.extend( {
                this.previous_values["vz"] = this.model.get("vz")[pindex]
                this.attributes_changed["vz"] =["vz"]
              }
+


I guess here is similar code needed if color is a 2d array, if sequence_index changes, it needs to update 'color'. Make sure the animation works for this.

Done, It didn't know that there was animation on the color!

maartenbreddels · 2017-03-03T14:23:07Z

js/src/volume.js

+  	        if(selected_previous.indexOf(i) != -1)
+  	            cur_color_previous = color_selected_previous
+     	        colors_previous.setXYZ(i, cur_color_previous[0], cur_color_previous[1], cur_color_previous[2]);
+  	    }



This will decrease the performance for having a million points, but a single color. So I'd rather see that if it is a single color, to not create an array.

maartenbreddels · 2017-03-03T14:23:59Z

js/src/volume.js

        if(!color_previous)
            color_previous = color;
+        console.log("color",color)
+        console.log("color_previous",color_previous)


Please remove these.

coveralls · 2017-03-03T14:55:51Z

Coverage remained the same at 86.47% when pulling b787a84 on jeammimi:multicolors into 852c259 on maartenbreddels:master.

coveralls · 2017-03-03T14:55:51Z

Coverage remained the same at 86.47% when pulling b787a84 on jeammimi:multicolors into 852c259 on maartenbreddels:master.

coveralls · 2017-03-03T14:55:51Z

Coverage remained the same at 86.47% when pulling b787a84 on jeammimi:multicolors into 852c259 on maartenbreddels:master.

coveralls · 2017-03-03T14:55:51Z

Coverage remained the same at 86.47% when pulling b787a84 on jeammimi:multicolors into 852c259 on maartenbreddels:master.

maartenbreddels · 2017-03-03T15:06:10Z

Could you fix the indenting a bit? For the rest it is working, animation etc? We could really use unittests at the js side now. But setting this up with opengl rendering won't be easy...

jeammimi · 2017-03-03T15:08:49Z

Yes unitest would be nice, but I have no clue how to implement them.
Yes everything I could think of testing worked. (And the animation with the color is really nice)
Do you want me to add the netbook I used for testing?

coveralls · 2017-03-03T16:18:04Z

Coverage remained the same at 86.47% when pulling ac132bf on jeammimi:multicolors into 852c259 on maartenbreddels:master.

coveralls · 2017-03-03T16:18:04Z

Coverage remained the same at 86.47% when pulling ac132bf on jeammimi:multicolors into 852c259 on maartenbreddels:master.

coveralls · 2017-03-03T16:18:04Z

Coverage remained the same at 86.47% when pulling ac132bf on jeammimi:multicolors into 852c259 on maartenbreddels:master.

coveralls · 2017-03-03T16:18:04Z

Coverage remained the same at 86.47% when pulling ac132bf on jeammimi:multicolors into 852c259 on maartenbreddels:master.

coveralls · 2017-03-03T17:21:30Z

Coverage remained the same at 86.47% when pulling fe7adff on jeammimi:multicolors into 852c259 on maartenbreddels:master.

coveralls · 2017-03-03T17:21:30Z

Coverage remained the same at 86.47% when pulling fe7adff on jeammimi:multicolors into 852c259 on maartenbreddels:master.

coveralls · 2017-03-03T17:21:30Z

Coverage remained the same at 86.47% when pulling fe7adff on jeammimi:multicolors into 852c259 on maartenbreddels:master.

coveralls · 2017-03-03T17:21:30Z

Coverage remained the same at 86.47% when pulling fe7adff on jeammimi:multicolors into 852c259 on maartenbreddels:master.

slinnarsson · 2017-03-03T17:39:39Z

Better than array of string would be numpy 2d vector of rgba values. Size (n_samples, 4). That would work out of the box with matplotlib's colormaps.

maartenbreddels · 2017-03-03T19:35:49Z

Yes, and another option would be to have a float->color mapping on the js side (color scales). Lets see if this would be work out:

shape is 0 dim, and it's a string, interpret as color
shape is 1 dim, items are strings, seperate color for each item
shape is 2 dim, items are strings, sequence of the above
no color scale / colormap is set
shape is 1 dim, items are floats, it should be of length 3 -> rgb values (possibly later 4 for opacity)
shape is 2 dim, items are float, it should be of shape (len(x), 3) -> rgb values
shape is 3 dim, items are float, it should be (sequence_length, len(x), 3) -> rgb values
color scale / colormap is set
- shape is 0 dim, item if float, use the colorscale to map to color
- shape is 1 dim, it's a seperate color for each item
- shape is 2 dim, again a sequence

So this can work out I think, no ambiguity about how color would be interpreted. The color scale is not something I want to address now. @jeammimi If you feel like implementing the rgb values by float, I'm happy to see this in this PR (hint: see _.isNumber, otherwise @slinnarsson can give it a try?

maartenbreddels · 2017-03-03T19:37:09Z

A different PR is fine as well, shall i accept this first?

jeammimi · 2017-03-06T08:02:01Z

I can give it a try. For the color scale, I am not familiar with it. So I think it will be for another time.
I will implement 1D 2D 3D with shape 3.
Transparency could be nice, but I don't know where it should be specified in the javascript part.

jeammimi · 2017-03-06T09:05:42Z

Ok it is done.
The only thing is that there is a warning that appears with traitlets when the array color is numeric.

traitlets/traitlets.py:565: FutureWarning: elementwise comparison failed; returning scalar instead, but in the future will perform elementwise comparison
  silent = bool(old_value == new_value)

jeammimi · 2017-03-06T09:08:15Z

So now it works for these cases:

shape is 0 dim, and it's a string, interpret as color
shape is 1 dim, items are strings, seperate color for each item
shape is 2 dim, items are strings, sequence of the above
shape is 1 dim, items are floats, it should be of length 3 -> rgb values
shape is 2 dim, items are float, it should be of shape (len(x), 3) -> rgb values
shape is 3 dim, items are float, it should be (sequence_length, len(x), 3) -> rgb values

coveralls · 2017-03-06T09:10:01Z

Coverage decreased (-4.1%) to 82.353% when pulling d0b0d73 on jeammimi:multicolors into 852c259 on maartenbreddels:master.

coveralls · 2017-03-06T09:10:01Z

Coverage decreased (-4.1%) to 82.353% when pulling d0b0d73 on jeammimi:multicolors into 852c259 on maartenbreddels:master.

maartenbreddels

Ok, thanks a lot for your contributions. I do have some comments, would be really great if you take this into account, they are important for maintenance and future development.

If you want you can share your name/twitter/email or whatever, so I could give you credits for the next release.

maartenbreddels · 2017-03-06T09:25:43Z

js/src/volume.js

+                //OD
+                    color =  to_rgb(variable)
+                    return color
+                }


I suggest refactoring this a bit, so it's easier to read/maintain.
instead of returning the color, and checking the color dim, you could do sth like this:

color = to_rgb(variable) return function(glyph_index) { return color }

maartenbreddels · 2017-03-06T09:27:23Z

js/src/volume.js

+
+                    for(var i = 0; i < max_count; i++) {
+                        tmp_color[i] = to_rgb(to_convert[i])
+                    }


And here:

... convert ... return function(glyph_index) { return tmp_color[glyph_index] }

etc..

maartenbreddels · 2017-03-06T09:44:15Z

js/src/volume.js

+            else{
+  	            var cur_color = color[i];
+            }
+


And then here you could use cur_color = color(i)

maartenbreddels · 2017-03-06T09:44:35Z

js/src/volume.js

+            }
+            else{
+                var cur_color_previous = color_previous[i];
+            }


and here cur_color_previous = color_previous(i)

maartenbreddels · 2017-03-06T09:56:47Z

js/src/volume.js

+              if (this.model.get("color") ) {
+                  color = this.model.get("color")
+                  if ( typeof color == "string" || typeof color[0] == "string" || typeof color[0][0] == "string") {
+                      if (!(typeof color == "string" || typeof color[0] == "string")){


Could you make this a bit simpler, sth like:

if ( ... 0 or 1d check ) { // don't do anything, we don't have a sequence, } else if ( .. 2d check ...) { // we have a sequence ... }

I think this is more easy to read.
Please be consistent with whitespace, no whitespace after if(,

maartenbreddels · 2017-03-06T09:57:10Z

js/src/volume.js

-        var color_previous = "color" in this.previous_values ? to_rgb(this.previous_values["color"]) : color;
+
+        function get_value_index_color(variable,index,max_count){
+            //Return a two D array (max_count,3)


I think this function should go top level, it can be reused if for instance we're gonna support lines. And instead of returning a color, I think it should return a function. (see the rest of the comments)
Could you document it, by starting with this list:

shape is 0 dim, and it's a string, interpret as color

shape is 1 dim, items are strings, seperate color for each item

shape is 2 dim, items are strings, sequence of the above

shape is 1 dim, items are floats, it should be of length 3 -> rgb values

shape is 2 dim, items are float, it should be of shape (len(x), 3) -> rgb values

shape is 3 dim, items are float, it should be (sequence_length, len(x), 3) -> rgb values

and then explaining how all these cases get handled. At the moment it's hard to say what is going on by quickly going over the code.

And please use this style (note the whitespace):

function get_value_index_color(variable, index, max_count) {

maartenbreddels · 2017-03-06T10:07:04Z

Lost of comments, sorry for that ;).
Color scale and transparency is another thing.. don't worry about it for the moment, at least we know that we have a list of unambigious behavious for 'scatter'.

Btw, we could think of doing unittests/test, but not on travis. Now it is possible to do a p3.savefig('test_multicolor.png') in the notebook, and compare say that image against an previous image to see we didn't mess up.
Maybe not yet for this PR, but just to let you know.

jeammimi · 2017-03-06T11:58:34Z

Ok, I modified everything accordingly.
The function get_value_index_color is now cleaner and easy to read I think.
The only thing I changed is that I don't create anymore the array tmp_color.
It is better for the memory, and I think that at worse the conversion is made twice.
Here is my email jeanmichel.arbona at gmail.com

coveralls · 2017-03-06T12:06:37Z

Coverage decreased (-4.3%) to 82.207% when pulling a4433a1 on jeammimi:multicolors into 852c259 on maartenbreddels:master.

coveralls · 2017-03-06T12:06:37Z

Coverage decreased (-4.3%) to 82.207% when pulling a4433a1 on jeammimi:multicolors into 852c259 on maartenbreddels:master.

coveralls · 2017-03-06T12:13:50Z

Coverage decreased (-4.3%) to 82.207% when pulling 18efca2 on jeammimi:multicolors into 852c259 on maartenbreddels:master.

coveralls · 2017-03-06T12:20:15Z

Coverage decreased (-4.3%) to 82.207% when pulling 18efca2 on jeammimi:multicolors into 852c259 on maartenbreddels:master.

maartenbreddels · 2017-03-06T12:24:54Z

Looking good 👍 , I'll pull this in.

jeammimi · 2017-03-06T12:38:32Z

Great!

maartenbreddels · 2017-03-06T12:47:04Z

I'll put something like this as an example:
https://www.astro.rug.nl/~breddels/test/wave/
I think it demonstates most of the features. Some arrays can be constant (x and z in this case), and y and color are a sequence.

maartenbreddels · 2017-03-06T14:02:17Z

I added size as well, it can be a float or a sequence.

jeammimi · 2017-03-06T14:03:32Z

Yes it is super nice.
Here the size is also a sequence no ? You implemented it ?

maartenbreddels · 2017-03-06T14:05:44Z

Yes, just pushed it to master.

slinnarsson · 2017-03-06T14:31:46Z

Fantastic! Thanks for doing this. The demo looks great. /Sten

…

-- Sten Linnarsson, PhD Professor of Molecular Systems Biology Karolinska Institutet Unit of Molecular Neurobiology Department of Medical Biochemistry and Biophysics Scheeles väg 1, 171 77 Stockholm, Sweden +46 8 52 48 75 77 (office) +46 70 399 32 06 (mobile)

On 6 Mar 2017, at 15:05, Maarten Breddels ***@***.***> wrote: Yes, just pushed it to master. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

jeammimi · 2017-03-06T14:35:04Z

I think there is a problem with the implementation for size.
I don't know why but when size is a float, the sequence does not work.

jeammimi · 2017-03-06T14:41:13Z

I think you should modify line 419 of volume.js with

if (this.model.get("size") && !_.isNumber(this.model.get("size")) && typeof this.model.get("size")[0][0] != "undefined" ) {

because if it is a number aparently it does not like being checked for an array

maartenbreddels · 2017-03-06T15:41:16Z

Yes, i fixed that, i'll push it to master.

maartenbreddels · 2017-03-06T15:42:03Z

fixed.

Multicolors - per glyph color

jeammimi added 5 commits March 2, 2017 14:16

Added multicolor

4811c6c

Merge branch 'master' into multicolors

7b9827a

Color can be string, list of string or 2D arrays of string

21f86ed

Reversed higher resolution

8cbb0ab

Removed log

b787a84

maartenbreddels mentioned this pull request Mar 3, 2017

Color and size per glyph #8

Closed

maartenbreddels reviewed Mar 3, 2017

View reviewed changes

Change 0D vs 1D and two D plus animation

ac132bf

Fix the indentation

fe7adff

Added rgba for color

d0b0d73

maartenbreddels reviewed Mar 6, 2017

View reviewed changes

CLeaner implementation

a4433a1

jeammimi added 2 commits March 6, 2017 13:02

Removed log

bc5fba3

Removed useless tmp variabel

18efca2

maartenbreddels merged commit 7386ab3 into widgetti:master Mar 6, 2017

mpu-creare pushed a commit to mpu-creare/ipyvolume that referenced this pull request Mar 26, 2018

Merge pull request widgetti#10 from jeammimi/multicolors

d0847b5

Multicolors - per glyph color

Multicolors #10

Multicolors #10

Conversation

jeammimi commented Mar 3, 2017

maartenbreddels left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Mar 3, 2017

coveralls commented Mar 3, 2017

coveralls commented Mar 3, 2017

coveralls commented Mar 3, 2017

maartenbreddels commented Mar 3, 2017

jeammimi commented Mar 3, 2017 • edited Loading

coveralls commented Mar 3, 2017

coveralls commented Mar 3, 2017

coveralls commented Mar 3, 2017

coveralls commented Mar 3, 2017

coveralls commented Mar 3, 2017

coveralls commented Mar 3, 2017

coveralls commented Mar 3, 2017

coveralls commented Mar 3, 2017

slinnarsson commented Mar 3, 2017

maartenbreddels commented Mar 3, 2017

maartenbreddels commented Mar 3, 2017

jeammimi commented Mar 6, 2017

jeammimi commented Mar 6, 2017

jeammimi commented Mar 6, 2017

coveralls commented Mar 6, 2017 • edited Loading

coveralls commented Mar 6, 2017

maartenbreddels left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maartenbreddels commented Mar 6, 2017

jeammimi commented Mar 6, 2017 • edited Loading

coveralls commented Mar 6, 2017 • edited Loading

coveralls commented Mar 6, 2017

coveralls commented Mar 6, 2017 • edited Loading

coveralls commented Mar 6, 2017 • edited Loading

maartenbreddels commented Mar 6, 2017

jeammimi commented Mar 6, 2017

maartenbreddels commented Mar 6, 2017

maartenbreddels commented Mar 6, 2017

jeammimi commented Mar 6, 2017 • edited Loading

maartenbreddels commented Mar 6, 2017

slinnarsson commented Mar 6, 2017 via email

jeammimi commented Mar 6, 2017

jeammimi commented Mar 6, 2017 • edited Loading

maartenbreddels commented Mar 6, 2017

maartenbreddels commented Mar 6, 2017

jeammimi commented Mar 3, 2017 •

edited

Loading

coveralls commented Mar 6, 2017 •

edited

Loading

jeammimi commented Mar 6, 2017 •

edited

Loading

coveralls commented Mar 6, 2017 •

edited

Loading

coveralls commented Mar 6, 2017 •

edited

Loading

coveralls commented Mar 6, 2017 •

edited

Loading

jeammimi commented Mar 6, 2017 •

edited

Loading

jeammimi commented Mar 6, 2017 •

edited

Loading