Ellipse parameter now supports major/minor axis lengths #1509

jlstevens · 2017-06-01T12:53:14Z

Implements a suggestion by @ea42gh that is backwards compatible with the old behavior. Needs testing and final discussion on whether this is useful before I add unit tests.

Edit: I'll also update the new element notebook for ellipse if we decide this is useful.

jbednar · 2017-06-01T13:25:45Z

holoviews/element/path.py

+            if major < minor:
+                raise ValueError('Minor axis length must be less than major axis length')
+            params['height'] = major
+        else:


Is it necessary to enforce this restriction? It's confusing if someone sets them the wrong way around, but e.g. if the values are controlled by interactive widgets, it can be tough to ensure that it doesn't get flipped in that way. You could even consider labelling them axis1 and axis2 for that reason, and suggesting that people use axis1 for the major axis.

Sure! @ea42gh also said something similar. Renaming to axis1 and axis2 would mean I would be happy to remove this restriction. If it is named 'major' and 'minor' then I really feel the latter should be smaller.

You could also simplify it and just use height and width, so that there would be no need to deprecate.

I had suggested this on gitter - the counter-argument was that these aren't really the correct names when discussing ellipses.

Naming parameters for geometrical objects is always difficult because the word that people use depends on things that aren't up to that parameter to decide. E.g. "height" and "width" are tricky for anything with an orientation, since rotating by 90 degrees will make the words seem backwards. But there's really no way around that, and the alternative of using names with no meaning at all (axis1) isn't any better, because then there's no way for people to guess which dimension will be controlled by which parameter. Given that there is already height, I'd suggest just keeping it and adding width; why not do the simplest thing, in a case where there is no truly great answer?

And of course even "Ellipse" is the wrong word if the height and width are equal, which is the default -- there's no way to win! So let's give up and do the simplest non-horrible thing.

On balance I probably agree and just adding width was my first instinct. Just having support for width and height as the two axes should be a huge improvement over having to use aspect. Even if that isn't quite the right terminology for this geometric shape, those concepts are general.

Sounds good.

That definitely would work well. The unnecessary restriction a<b would just force user code to guard against it, so I would definitely argue against it.

jbednar · 2017-06-01T14:37:16Z

holoviews/element/path.py

+    width = param.Number(default=1, doc="The width of the ellipse.")
+
+    aspect= param.Number(default=1.0, doc="""
+       Final divisor only applied to the final height.""")



The code seems to apply it to the width, not the height? I think this could be explained more clearly.

Well spotted! Fixed in 3c25447.

Well, but in the case where aspect is useful, the width is initially set from the provided height. So it's still confusing.

I probably wouldn't recommend using aspect anymore tbh and would consider deprecating it. I am only leaving it in right now for backwards compatibility. Should I issue a deprecation warning when it is used?

I think aspect is very useful. In some situations one wants to control height and width explicitly (e.g. to enclose a certain area), and in others one wants to control the shape and size separately (with aspect setting the shape, and height setting the size). Neither case is more valid than the other.

Well, but in the case where aspect is useful, the width is initially set from the provided height. So it's still confusing.

How would you like it to behave? I can't change how it works currently as that would break backwards compatibility but we could change the behavior for HoloViews 2.0.

Oh, I'm not arguing for the code to change, only for the docstring to reflect what the code does.

jbednar · 2017-06-01T20:12:58Z

Looks good to me, thanks!

philippjfr · 2017-06-02T01:29:09Z

holoviews/element/path.py

@@ -148,7 +149,11 @@ def clone(self, *args, **overrides):
        settings = dict(self.get_param_values(), **overrides)
        if not args:
            settings['plot_id'] = self._plot_id
-        return self.__class__(*args, **settings)
+
+        pos_args = getattr(self, '_' + type(self).__name__ + '__pos_params', [])


Is this necessary? Why would self.__pos_params work?

Why wouldn't it work? Name mangling due to the double underscore.

Oh, instead of getattr? In this case it might work...

Nope just checked, name mangling is still an issue.

Might not work actually, for that reason a single underscore may have been cleaner. There's a bunch of them now though, so up to you.

I believe single underscore would have messed up Layouts and AttrTrees. It was done like this for a reason but I can't quite remember all the details.

I partially remember - clone has to use getattr as not all elements need the positional parameters declared (most don't).

Calling getattr will create a path on AttrTrees resulting in an annoying entry which doesn't happen for double underscored entries. You have to be careful with getattr when AttrTrees are involved...

jbednar · 2017-06-02T14:59:23Z

holoviews/element/path.py

+
+    aspect= param.Number(default=1.0, doc="""
+       Optional multiplier applied to the box length to compute the
+       width in cases where only the length value is set.""")


length -> height ?

The first argument corresponds to the width - the problem is that saying you compute the width from the width and aspect is confusing. I wanted the equivalent of 'diameter' for squashing a circle with the aspect. Box 'size' maybe?

Tried using 'size' instead in 30beb5c

Looks clearer, thanks.

jlstevens · 2017-06-02T14:59:43Z

@philippjfr @jbednar I think I am now happy with the API and behavior of these shapes. If you agree I'll add some unit tests, update the element notebooks and we can merge!

jbednar · 2017-06-02T15:00:16Z

holoviews/element/path.py

+    A circle is a degenerate ellipse where the width and height are
+    equal. To specify these explicitly, you can use:
+
+    Ellipse(x,y, (height, width))


(width, height)

Thanks. Fixed in 30beb5c.

jbednar

Looks good!

philippjfr · 2017-06-02T15:07:42Z

Looks good to me as well. Happy to merge after adding some tests.

jlstevens · 2017-06-02T20:02:29Z

I've updated the element notebooks. As soon as I've added a few unit tests I think this PR will be ready!

philippjfr · 2017-06-03T21:52:17Z

examples/elements/bokeh/Box.ipynb

    "data = np.sin(np.mgrid[0:100,0:100][1]/10.0)\n",
-    "data[range(30, 70), range(30, 70)] = -3\n",
-    "hv.Image(data) * hv.Box(-0, 0, 0.5 )"
+    "data[range(40, 60) + range(40,50), range(20, 40)+ range(70,80)] = -3\n",


Adding ranges will fail in py3.

Good point. What would you recommend for a py2/py3 solution?

Not sure tbh, np.arange but then you have to concatenate, list is also ugly.

Fixed in 9079666

jlstevens · 2017-06-04T20:46:28Z

@philippjfr Ready to merge when the tests go green.

philippjfr · 2017-06-04T23:37:31Z

Looks good. Merging.

jlstevens added this to the v1.8 milestone Jun 1, 2017

jbednar reviewed Jun 1, 2017

View reviewed changes

philippjfr reviewed Jun 2, 2017

View reviewed changes

jlstevens force-pushed the ellipse_improvement branch from 6963169 to b312be2 Compare June 2, 2017 14:58

jbednar reviewed Jun 2, 2017

View reviewed changes

jbednar approved these changes Jun 2, 2017

View reviewed changes

philippjfr reviewed Jun 3, 2017

View reviewed changes

jlstevens and others added 11 commits June 4, 2017 20:58

Ellipse parameter now supports major/minor axis lengths

ef12996

Fixed typo in docstring

880e5d9

Simplified Ellipse to support a straightforward width parameter

90bda0c

Fixed incorrect docstring

f3f24b0

Updated docstring and restricted aspect to the height-only spec

a539a37

Fixed clone method for BaseShapes

8e0a7c1

Further fixes and improvements to Ellipse

3b02880

Made the Box element's API consistent with Ellipse

01fd33a

Fixed outdated docstrings

86c11fd

Updated Ellipse element notebooks

85ee24d

Updated Box and Ellipse element notebooks

3fefa32

jlstevens force-pushed the ellipse_improvement branch from 3ebcbbd to 3fefa32 Compare June 4, 2017 20:00

jlstevens added 2 commits June 4, 2017 21:38

Added seven unit tests of Ellipse and Box

e2df3ee

Made Box element notebooks Python 3 compatible

9079666

philippjfr merged commit e0358d4 into master Jun 4, 2017

philippjfr deleted the ellipse_improvement branch June 17, 2017 17:04

Ellipse parameter now supports major/minor axis lengths #1509

Ellipse parameter now supports major/minor axis lengths #1509

Conversation

jlstevens commented Jun 1, 2017 • edited Loading

jbednar Jun 1, 2017 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbednar Jun 1, 2017 • edited Loading

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

jbednar commented Jun 1, 2017

Choose a reason for hiding this comment

jlstevens Jun 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philippjfr Jun 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlstevens Jun 2, 2017 • edited Loading

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

jlstevens commented Jun 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbednar left a comment

Choose a reason for hiding this comment

philippjfr commented Jun 2, 2017

jlstevens commented Jun 2, 2017 • edited Loading

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

jlstevens commented Jun 4, 2017

philippjfr commented Jun 4, 2017

jlstevens commented Jun 1, 2017 •

edited

Loading

jbednar Jun 1, 2017 •

edited

Loading

jbednar Jun 1, 2017 •

edited

Loading

jlstevens Jun 2, 2017 •

edited

Loading

philippjfr Jun 2, 2017 •

edited

Loading

jlstevens Jun 2, 2017 •

edited

Loading

jlstevens commented Jun 2, 2017 •

edited

Loading