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

Ellipse parameter now supports major/minor axis lengths #1509

Merged
merged 13 commits into from Jun 4, 2017

Conversation

Projects
None yet
4 participants
@jlstevens
Member

jlstevens commented Jun 1, 2017

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.

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

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

This comment has been minimized.

@jbednar

jbednar Jun 1, 2017

Member

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.

This comment has been minimized.

@jlstevens

jlstevens Jun 1, 2017

Member

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.

This comment has been minimized.

@jbednar

jbednar Jun 1, 2017

Member

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

This comment has been minimized.

@jlstevens

jlstevens Jun 1, 2017

Member

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

This comment has been minimized.

@jbednar

jbednar Jun 1, 2017

Member

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?

This comment has been minimized.

@jbednar

jbednar Jun 1, 2017

Member

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.

This comment has been minimized.

@jlstevens

jlstevens Jun 1, 2017

Member

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.

This comment has been minimized.

@jbednar

jbednar Jun 1, 2017

Member

Sounds good.

This comment has been minimized.

@ea42gh

ea42gh Jun 1, 2017

Contributor

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.

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.""")

This comment has been minimized.

@jbednar

jbednar Jun 1, 2017

Member

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

This comment has been minimized.

@jlstevens

jlstevens Jun 1, 2017

Member

Well spotted! Fixed in 3c25447.

This comment has been minimized.

@jbednar

jbednar Jun 1, 2017

Member

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

This comment has been minimized.

@jlstevens

jlstevens Jun 1, 2017

Member

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?

This comment has been minimized.

@jbednar

jbednar Jun 1, 2017

Member

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.

This comment has been minimized.

@jlstevens

jlstevens Jun 1, 2017

Member

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.

This comment has been minimized.

@jbednar

jbednar Jun 1, 2017

Member

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

@jbednar

This comment has been minimized.

Member

jbednar commented Jun 1, 2017

Looks good to me, thanks!

@@ -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', [])

This comment has been minimized.

@philippjfr

philippjfr Jun 2, 2017

Member

Is this necessary? Why would self.__pos_params work?

This comment has been minimized.

@jlstevens

jlstevens Jun 2, 2017

Member

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

This comment has been minimized.

@jlstevens

jlstevens Jun 2, 2017

Member

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

This comment has been minimized.

@jlstevens

jlstevens Jun 2, 2017

Member

Nope just checked, name mangling is still an issue.

This comment has been minimized.

@philippjfr

philippjfr Jun 2, 2017

Member

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.

This comment has been minimized.

@jlstevens

jlstevens Jun 2, 2017

Member

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.

This comment has been minimized.

@jlstevens

jlstevens Jun 2, 2017

Member

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...

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.""")

This comment has been minimized.

@jbednar

jbednar Jun 2, 2017

Member

length -> height ?

This comment has been minimized.

@jlstevens

jlstevens Jun 2, 2017

Member

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?

This comment has been minimized.

@jlstevens

jlstevens Jun 2, 2017

Member

Tried using 'size' instead in 30beb5c

This comment has been minimized.

@jbednar

jbednar Jun 2, 2017

Member

Looks clearer, thanks.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Jun 2, 2017

@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!

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))

This comment has been minimized.

@jbednar

jbednar Jun 2, 2017

Member

(width, height)

This comment has been minimized.

@jlstevens

jlstevens Jun 2, 2017

Member

Thanks. Fixed in 30beb5c.

@jbednar

jbednar approved these changes Jun 2, 2017

Looks good!

@philippjfr

This comment has been minimized.

Member

philippjfr commented Jun 2, 2017

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

@jlstevens

This comment has been minimized.

Member

jlstevens commented Jun 2, 2017

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

"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",

This comment has been minimized.

@philippjfr

philippjfr Jun 3, 2017

Member

Adding ranges will fail in py3.

This comment has been minimized.

@jlstevens

jlstevens Jun 3, 2017

Member

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

This comment has been minimized.

@philippjfr

philippjfr Jun 3, 2017

Member

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

This comment has been minimized.

@jlstevens
@jlstevens

This comment has been minimized.

Member

jlstevens commented Jun 4, 2017

@philippjfr Ready to merge when the tests go green.

@philippjfr

This comment has been minimized.

Member

philippjfr commented Jun 4, 2017

Looks good. Merging.

@philippjfr philippjfr merged commit e0358d4 into master Jun 4, 2017

4 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.009%) to 79.3%
Details
s3-reference-data-cache Test data is cached.
Details

@philippjfr philippjfr deleted the ellipse_improvement branch Jun 17, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment