fix: extra quote + enh: control over line visibility #72

satra · 2017-09-24T04:24:47Z

No description provided.

coveralls · 2017-09-24T04:40:50Z

Changes Unknown when pulling 93e3f18 on satra:enh/line-visibility-control into ** on maartenbreddels:master**.

coveralls · 2017-09-27T14:47:26Z

Changes Unknown when pulling 8a94290 on satra:enh/line-visibility-control into ** on maartenbreddels:master**.

satra · 2017-09-27T14:55:00Z

@maartenbreddels - it would be awesome if you can review/merge this. the pattern itself can be extended to the other plot routines.

we are trying to simultaneously work on a set of domain specific widgets that depends on this.

maartenbreddels · 2017-09-27T15:03:46Z

Hi Satrajit,

thanks for your contributions. What about making this arguments to plot()?

cheers.

Maarten

satra · 2017-09-27T15:36:34Z

@maartenbreddels - it seems like a decision as to where to bring up all these arguments. so if every ipv.Scatter argument was an argument to plot then it would be fine to make everything arguments to plot. i just didn't know what you were thinking w.r.t the general api.

another option is to call kwargs, scatter_kwargs then you won't run into the issue of replicating things whenever scatter changes.

maartenbreddels · 2017-09-27T17:49:33Z

I think every property of Scatter that makes sense for a function in pylab should be an argument, and kwargs should be used for something that doesn't make sense, but that you may want to set anyway.

What would renaming scatter_kwargs to kwargs do, can you explain the issue?

satra · 2017-09-27T18:41:26Z

@maartenbreddels - i updated it i think to reflect what you are saying.

however, i also made some changes to the other scatter wrapper functions. mostly this has to do with consistency rather than domain conventions.

satra · 2017-09-27T18:42:19Z

if domain conventions should be used, for-example marker instead of geo, perhaps the signature of scatter should be updated? i have no strong feelings either way on this.

maartenbreddels · 2017-09-28T10:54:54Z

Great work. I don't think the argument have to match one-to-one match what is in Scatter, but if they are useful the should be exposed. I'll give some feedback on the PR, that will make it clearer maybe I mean, great work 👍

maartenbreddels

So the main question I have is whether we should have plot and scatter have the same argument, but different defaults, what do you think?

maartenbreddels · 2017-09-28T10:55:49Z

ipyvolume/pylab.py

@@ -367,12 +367,12 @@ def plot(x, y, z, color=default_color, visible_lines=True, color_selected=None,
    :param y: {y}
    :param z: {z}
    :param color: {color}
-    :param visible_lines: {visible_lines}


I'd only expose 'visible'.

maartenbreddels · 2017-09-28T10:58:02Z

ipyvolume/pylab.py

@@ -367,12 +367,12 @@ def plot(x, y, z, color=default_color, visible_lines=True, color_selected=None,
    :param y: {y}
    :param z: {z}
    :param color: {color}
-    :param visible_lines: {visible_lines}
-    :param color_selected: {color_selected}


This arg would only make sense for scatter, unless we want plot and scatter to have the same arguments but different defaults, that may not be such a bad idea actually.

maartenbreddels · 2017-09-28T10:59:13Z

ipyvolume/pylab.py

@@ -402,7 +402,7 @@ def scatter(x, y, z, color=default_color, size=default_size,
    :param size: {size}
    :param size_selected: like size, but for selected glyphs
    :param color_selected:  like color, but for selected glyphs
-    :param geo: {geo} Uses marker


I'd say marker, in the Scatter widget I may also change this to marker.

maartenbreddels · 2017-09-28T11:00:15Z

ipyvolume/pylab.py

-    :param vx: {x_dir}
-    :param vy: {y_dir}
-    :param vz: {z_dir}
+    :param vx: x_dir


u,v,w is more consistent with matlab https://nl.mathworks.com/help/matlab/ref/quiver3.html and matplotlib https://matplotlib.org/mpl_toolkits/mplot3d/tutorial.html#line-plots

satra · 2017-09-28T12:46:45Z

@maartenbreddels

So the main question I have is whether we should have plot and scatter have the same argument, but different defaults, what do you think?

kind of. perhaps the other way to state it is:

any argument of scatter that is set by plot/quiver/scatter should be exposed as a specific keyword in the definition of plot/quiver/scatter otherwise they cannot be modified by the user using kwargs.

i originally ran into it since i wanted to turn visibility off, i was drawing 5000+ lines of different lengths and the update was really slow.

i reverted some of those matlab/matplotlib names for now. i think it would be good to make scatter consistent with it.

…ty-control * upstream/master: Extra " removed

maartenbreddels · 2017-09-28T12:58:28Z

any argument of scatter that is set by plot/quiver/scatter should be exposed as a specific keyword in the definition of plot/quiver/scatter otherwise they cannot be modified by the user using kwargs
i originally ran into it since i wanted to turn visibility off, i was drawing 5000+ lines of different lengths and the update was really slow.

Can you explain this, I'm not sure I follow you. Any argument not exposed by plot can simply be picked up by kwargs and passed on right?

satra · 2017-10-08T17:30:05Z

@maartenbreddels - sorry this must have skipped my notification

Can you explain this, I'm not sure I follow you. Any argument not exposed by plot can simply be picked up by kwargs and passed on right?

not if that arg is defined as a separate keyword arg in the inner func. the reason pull-request is in place is because i was trying to set visible_lines=False in plot. however, because of the error noted below i could not.

hence the two options are to always expose any settable keyword that can be manipulated at both levels, but default it in the outer level. hope that makes sense.

def inner_func(a=1, b=2, c=3):
    pass

def outer_func(**kwargs):
    inner_func(a=2, **kwargs)

outer_func(a=3)

results in:

---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-1-81cbd0310f74> in <module>()
      5     inner_func(a=2, **kwargs)
      6 
----> 7 outer_func(a=3)

<ipython-input-1-81cbd0310f74> in outer_func(**kwargs)
      3 
      4 def outer_func(**kwargs):
----> 5     inner_func(a=2, **kwargs)
      6 
      7 outer_func(a=3)

TypeError: inner_func() got multiple values for keyword argument 'a'

satra · 2017-10-11T11:17:20Z

@maartenbreddels - just a ping here in case you didn't get a chance to look at my response.

…ty-control * upstream/master: Update README.md fix: plot_isosurface was calling marching cubes twice, and better steps for sliders support for jupyter lab 0.27

maartenbreddels · 2017-10-11T11:39:51Z

Ok, I understand the issue better.
My feeling on this, is that for plotting a line you want different default than scatter, but not directly expose them as defaults in the function arguments (I mean, having connected=True is a good default, and it makes no sense to have connected=True in the function arguments as default, and on top of that you need to write a docstring to explain this).
However, I do recognise that there are 'crazy' situations where you do actually want to override these default.
What about the following pattern (simplified for clarity):

def plot(x, y, z, color='red', **kwargs):
  defaults = dict(connected=True, visible_markers=False)
  kwargs = dict(defaults, **kwargs)  # merge dicts, where kwargs get preference over defaults
  scatter = Scatter(x, y, z, color=color, **kwargs)

So the different defaults (the ones that make sense to hide, such as connected and visible_markers are kind of hidden, but possible to override.

satra · 2017-10-11T12:02:31Z

@maartenbreddels - ah that was the previous proposal :)

see this deleted version here:

782aec3#diff-f865c2c721a2719a6169fd34dfb51bc5L373

i'm happy to change it back to that state.

coveralls · 2017-10-11T12:03:05Z

Coverage remained the same at 61.021% when pulling 8499c8c on satra:enh/line-visibility-control into 771b769 on maartenbreddels:master.

satra · 2017-10-11T12:13:12Z

@maartenbreddels - made the adjustments.

coveralls · 2017-10-11T12:57:13Z

Coverage decreased (-0.2%) to 60.842% when pulling 4ac044c on satra:enh/line-visibility-control into 771b769 on maartenbreddels:master.

coveralls · 2017-10-11T15:42:38Z

Coverage increased (+0.5%) to 61.538% when pulling 16fa729 on satra:enh/line-visibility-control into 771b769 on maartenbreddels:master.

maartenbreddels

Excellent 👍 , thanks for putting in unittests as well. Some minor comments, hope you don't mind the endless dragging, but I'd like to get this right.

maartenbreddels · 2017-10-13T06:23:05Z

ipyvolume/pylab.py

+    defaults = dict(visible_lines=True, color_selected=None, size_selected=1,
+                    size=1, connected=True, visible_markers=False)
+    kwargs = dict(defaults, **kwargs)
+    scatter_hdl = ipv.Scatter(x=x, y=y, z=z, color=color, **kwargs)


Why rename do scatter_hdl? I don't know what hdl means.

two reasons -

while it technically works there is a name clash, scatter is a method as well. so there is a local vs global clash that i do not like.

_hdl stands for "handle" since it's really a pointer to the object.

fair point, but I don't abrvs that aren't std ;). I'd rather see s = ipv.Scatter(..), and for python it's always by reference so _hdl doesn't add information, except indeed preventing a possible name collision. my preference goes to s or scatter_object

maartenbreddels · 2017-10-13T06:23:51Z

ipyvolume/pylab.py

@@ -389,20 +392,26 @@ def scatter(x, y, z, color=default_color, size=default_size, size_selected=defau
    :param size: {size}
    :param size_selected: like size, but for selected glyphs
    :param color_selected:  like color, but for selected glyphs
-    :param marker: {marker}
+    :param marker: Uses marker


Why replace this docstring (which is substituted by the snippets on top) by this?

sorry that's part of an older segment of changes - will revert that

maartenbreddels · 2017-10-13T06:25:00Z

ipyvolume/pylab.py

-                             size_selected=size_selected, geo=marker, selection=selection, **kwargs)
-    fig.scatters = fig.scatters + [scatter]
-    return scatter
+    if 'geo' in kwargs:


'geo' is used for the Scatter object, but I think marker is a better name, and for this API i chose marker, which is taking it's role.

So I'd remove this part, and simply use marker.

since under the current formulation, kwargs go on to the scatter call, if someone does use geo it will result in a clash, since marker gets converted to geo. this simply says if some puts in geo it will overwrite marker

Lets us remove this, and change to use geo->marker (or leave that for me to do)

maartenbreddels · 2017-10-13T06:25:48Z

ipyvolume/pylab.py

+    if 'geo' in kwargs:
+        marker = kwargs['geo']
+        del kwargs['geo']
+    scatter_hdl = ipv.Scatter(x=x, y=y, z=z, color=color, size=size,


Same applies here, why hdl?

maartenbreddels · 2017-10-13T06:26:10Z

ipyvolume/pylab.py


 @_docsubst
-def quiver(x, y, z, u, v, w, size=default_size * 10, size_selected=default_size_selected * 10, color=default_color,
+def quiver(x, y, z, u, v, w, size=default_size * 10,
+           size_selected=default_size_selected * 10, color=default_color,


I should be running flake8 :)

maartenbreddels · 2017-10-13T06:26:53Z

ipyvolume/pylab.py

-    return scatter
+    if 'geo' in kwargs:
+        marker = kwargs['geo']
+        del kwargs['geo']


Same here, please just use marker and forget about geo. I think maybe we should change Scatter.geo to Scatter.marker.

if you changed Scatter.geo to Scatter.marker we can simplify some of this.

maartenbreddels · 2017-10-13T06:28:08Z

ipyvolume/pylab.py

+        marker = kwargs['geo']
+        del kwargs['geo']
+    if 'vx' in kwargs or 'vy' in kwargs or 'vz' in kwargs:
+        raise KeyError('Please use u, v, w instead of vx, vy, vz')


I don't think this should be added, quiver had never vx etc as arguments, is we were doing some backward incompatible tricks this would make sense though.

same reasoning as before:

quiver(..., u=u, ..., vx=u) would be legal but bad. this just raises an exception to notify the user. all of this stems from the fact that any kwargs eventually gets passed to the lowest level scatter call.

good point, indeed!

maartenbreddels · 2017-10-13T06:28:28Z

ipyvolume/pylab.py

+        del kwargs['geo']
+    if 'vx' in kwargs or 'vy' in kwargs or 'vz' in kwargs:
+        raise KeyError('Please use u, v, w instead of vx, vy, vz')
+    scatter_hdl = ipv.Scatter(x=x, y=y, z=z, vx=u, vy=v, vz=w,


Here as well, please remove the _hdl suffix.

satra · 2017-10-13T11:48:06Z

@maartenbreddels - updated, if things look you can merge and then do the marker/geo bit. i'll be offline for a bit.

maartenbreddels · 2017-10-13T11:53:19Z

Excellent, thanks a lot, and thanks for your patience! 👍

coveralls · 2017-10-13T11:58:38Z