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

Color array when rgb can be transferred with binary transfer #16

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

jeammimi
Copy link
Contributor

I added the possibility to transfer color with the binary transfer when it is an rgb an array of rgb
or an array of array.
I think that the next one I would like to implement is to transfer with the binary transfer the hexadecimal representation of the color. (I like it better because it is a lot smaller when saving it for the embedded version)

@coveralls
Copy link

coveralls commented Mar 15, 2017

Coverage Status

Coverage decreased (-0.3%) to 78.46% when pulling 3377236 on jeammimi:color into 65300b5 on maartenbreddels:master.

@coveralls
Copy link

coveralls commented Mar 15, 2017

Coverage Status

Coverage decreased (-0.3%) to 78.46% when pulling 827be3f on jeammimi:color into 65300b5 on maartenbreddels:master.

@coveralls
Copy link

coveralls commented Mar 15, 2017

Coverage Status

Coverage decreased (-0.3%) to 78.46% when pulling ded32bc on jeammimi:color into 65300b5 on maartenbreddels:master.

@maartenbreddels
Copy link
Collaborator

I'd like to do the deserialize of the color seperately, I think I'll take some of this code and refactor it, instead of merging it... I'll let you know.

@jeammimi
Copy link
Contributor Author

Ok.

I did it that way because now you can be in performance 1 all the time,
and It will send the array it can in a fast way, and the other in the regular way.
It works for color, as well as if you decide to send x with different number of point for different frame.

I modified the fact that you were forcing the conversion to float32.
I think it is important to keep the type of array the same type that is send
to the widget, because for example I was sending integer, to make the
embeded version lighter.

I was thinking trying to make the embeding work when the performance is set to 1, and also look
into how to save the data in a separate file.

@maartenbreddels
Copy link
Collaborator

Ok, sounds good. I'm working on the performance of create_mesh, since that is a bit slow, and difficult to maintain. For the embedding with performance = 1, I'm not sure you should attack that now. For the more general case, with performance = 2, there is quite some progress here: jupyter-widgets/ipywidgets#1194
I think by making use of the functions on the js side it will save quite some work, but maybe you can do the Python side?

@jeammimi
Copy link
Contributor Author

I am sory, I am not sure what you mean by

I think by making use of the functions on the js side it will save quite some work, but maybe you can do the Python side?

@jeammimi
Copy link
Contributor Author

Have you made a comparison of performance 1 and 2 for the speed?

@maartenbreddels
Copy link
Collaborator

If you do from the menu in the notebook, Widget->Embed widget. This is the javascript side making a json serialization. If you do the ipyvolume.pylab.save, it is the python side doing the serialization.

@maartenbreddels
Copy link
Collaborator

I did a mayor refactor, but I think your changes are included, please check if everything is working as expected, so I can close this.

@maartenbreddels
Copy link
Collaborator

Make sure you run npm update btw, threejs required a new version.

@jeammimi
Copy link
Contributor Author

Not everything was included, but I can create another pull request.
One thing does not work anymore:
Before I was able to send array of hexadecimal value for the color such as:
array(['#f00', '#ff0a00', '#ff1500', '#ff1f00', '#ff2900', '#ff3400'])
now It does not work anymore. The problem is that for numpy it is a u7 type,
that now is included in the performance 1 case but should not. I think we shourd keep the list
of known type as it is, and this one should be json serialised.
One "improvement" is missing :
I think when using the union in traitlest (for example for color and size) we should put the array before the unicode (for color) and array before float (for size). The reason is that it first try the unicode case, and if it can not convert it, send a representation of the object in a string (I guess it is for debugging purpose) doing the repr of a numpy array is ok because it is truncated, but if for some reason we send a list, it is going to do a repr of a list which could be 1000x1000x3 (It happend to me for the color because I did not convert it to a numpy array). And doing the repr of such a list can take 1-2 seconds.
So putting the array first get rid of this problem

@maartenbreddels
Copy link
Collaborator

Hmm, surprised by that, this line only allows for ints and floats, the rest should go as json (for performance == 1). Let me think about the order of the traitlets, I noticed it in the diff, but didn't think it was intentional.

@jeammimi
Copy link
Contributor Author

For the traitlets this might convince you:

%timeit -n 1 p3.scatter(x=np.array([1,1]),y=np.array([1,1]),z=np.array([1,1]),\
                        size=1,color=np.random.rand(1000*1000*3).reshape(1000,1000,3))

1 loop, best of 3: 87.8 ms per loop

%timeit -n 1 p3.scatter(x=np.array([1,1]),y=np.array([1,1]),z=np.array([1,1]),\
                        size=1,color=np.random.rand(1000*1000*3).reshape(1000,1000,3).tolist())

1 loop, best of 3: 6.31 s per loop

The second one is very slow, and if you change the position of array, before unicode, you wont see this

@jeammimi
Copy link
Contributor Author

For the first point , the line you are mentionning it allows all u element and the array I mentionned is
u7, and should not be fast serialized.

@maartenbreddels
Copy link
Collaborator

u is unsigned integer U is unicode, quite confusing, isn't it?

>>> a = np.array(['#f00', '#ff0a00', '#ff1500', '#ff1f00', '#ff2900', '#ff3400'])
>>> a.dtype, a.dtype.kind
(dtype('<U7'), 'U')

Thanks for the report on the Unicode(), it must be in the error handling code.

@jeammimi
Copy link
Contributor Author

I think the same problem is true for size size_selected and color_selected.

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

Successfully merging this pull request may close these issues.

3 participants