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

Simpler option setting #222

Closed
jbednar opened this issue Jul 21, 2015 · 22 comments
Closed

Simpler option setting #222

jbednar opened this issue Jul 21, 2015 · 22 comments
Labels
tag: API type: feature A major new feature
Milestone

Comments

@jbednar
Copy link
Member

jbednar commented Jul 21, 2015

Is there a fundamental reason why we can't do:

hv.Points(data)(color='red'})

instead of:

hv.Points(data)(style={'color':"red"})

I.e., does the distinction between the different types of options need to be something the user worries about in practice? Couldn't holoviews just take a single list of keyword options and apply it to the plot options, style options, and normalization options in order, giving an error if none of those three were to consume it? It's unlikely that any of the normalization options will ever match the name of one of the other two types, but of course it's possible that there is a style option that has the same name as one of the plot options. Even so, (a) if that happens it's going to be very confusing for our users no matter what we do, so it's presumably good to avoid such name clashes, and (b) as long as we clearly define which option type has priority, people can deal with this issue for the very rare cases when it comes up, without having to use such a wordy and awkward syntax the rest of the time. What do you think?

@jbednar jbednar added type: feature A major new feature tag: API labels Jul 21, 2015
@jlstevens
Copy link
Contributor

I can certainly see your suggestion would be convenient (for style options at least) and I would love a more succinct syntax!

The issue is we've designed our option systems to have these different groups (style, plot and norm by default) and they act a little bit like namespaces for options. I agree that ideally keys shouldn't clash but I don't think that means we should get rid of the idea and put all our keywords into a 'global' option namespace. Putting things into a common namespace for convenience makes me think of from pylab import * which we agree isn't a good idea...

So I am in favor of an easier syntax but allowing style, plot and normalization options to mix like this could cause trouble and difficulties down the line (the way global names tend to!). Here is one suggestion I might consider: the __call__ method knows the Element it is on and therefore could look up the corresponding style/plot/norm options via the Store. This means maybe you could drop style= or plot= if we can infer that all the keywords are either style or plot appropriately.

In other words, as long as you are only specifying one type of option, we could drop the need for the keywords, but if you want to specify a mixture, you still need to explicitly state what type of group they are in as before. I think this might be a plausible compromise that is safer than putting everything into the same namespace - although I should say that I haven't fully convinced myself that this is a good idea yet!

@jbednar
Copy link
Member Author

jbednar commented Jul 24, 2015

I'm not proposing to eliminate the distinction between the groups in general; indeed, they do act like namespaces, and the distinction is useful (especially to be able to tell which options are available for all backends, and which are backend-specific). And in IPython there's a succinct syntax already, which is great. So I'm not arguing for pooling all options into a single namespace. Instead, I'm just arguing that the pure-Python user doesn't always need to worry about the distinction, just like one can always do 'import *'. Thus, if the user wants to do hv.Points(data)(style={'color':"red"}), great -- namespace is respected. But as shortcut, if the user does hv.Points(data)(color='red'}), it could still work and be quite well defined, with vastly less potential for clashes than "import *" (since the number of backends will surely always be a small countable number). In obscure cases this will do something confusing (i.e., when the user means to set a style option for which there is a corresponding plot option with the same name), but this will be very rare, and in all other cases it should be much more convenient.

BTW, I'm less convinced that the distinction between plot options and normalization options is useful; I can't think of any case where one would want to separate those two categories. Seems like just two more plot options, to me.

@philippjfr
Copy link
Member

I'd be happy with this change. There generally shouldn't be any clashes between option groups and we could warn if you're attempting to specify an option that is duplicated across groups. It does go against explicit is better than implicit but I think it's sufficient to warn if there are clashes since in general we would generally advise against duplicating option keywords, simply to avoid confusion.

@jbednar
Copy link
Member Author

jbednar commented Jul 25, 2015

Warning if it's ambiguous sounds like a very good solution to me; still convenient but they know that they should be more explicit in that case.

@jlstevens
Copy link
Contributor

If we weren't able to check the sets of keywords in option group, I would definitely be against the idea. However, as we can check the keywords in each option group, we can also ensure that there is no ambiguity in what the user supplies. If we do it this way, I am happy with the overall suggestion although I would prefer to raise an exception instead of issuing a warning: I feel the user should be made to use the explicit syntax in the (rare!) cases where the supplied name is ambiguous.

We will also have to immediately raise an exception if the user tries to use this simplified syntax and mistypes a keyword name: the keyword won't be recognized so there is no way of knowing which group it is supposed to belong to. Note that this feature would (at least initially) be implemented only for __call__ and only on basic Elements (i.e not for containers): the option specification syntax is complicated enough as we already support a huge number of different formats.

@jbednar
Copy link
Member Author

jbednar commented Jul 25, 2015

I'd be in favor of reducing the number of different option-setting formats supported, as long as there is one completely explicit way, plus this convenient way. I think we went a little bit crazy with all the permutations currently supported. Even if we don't want to break the API at this point, we could certainly avoid documenting all but one of the other ways, to reduce confusion, being sure to remove all such examples from the tutorials.

Be careful about making option setting raise exceptions, though -- when people switch backends, we want things to keep working as best they can -- getting some plot out is much, much better than getting an error message. Following the HTML/CSS metaphor that we proposed in the recent paper, we wouldn't even raise such warnings visibly (as browsers are required just ignore stylistic hints that they don't know what to do with), but that's a bit extreme. We could raise an exception for options not defined for any backend, but even that's not necessarily a good idea -- e.g. it prevents forward compatibility, with the same plot working fine across various HoloViews versions apart from some small stylistic hint that was optional anyway but someone happened to specify because they had a newer HoloViews version than their colleagues. I'm inclined to think that all option settings should just generate warnings (unless the user explicitly turns on some warnings_as_exceptions flag, e.g. for vetting or debugging), and that the warnings should always be silenceable (so that we could easily put in the right hints for both Matplotlib and Bokeh backends without having to put in lots of special-case code).

@jlstevens
Copy link
Contributor

Be careful about making option setting raise exceptions, though -- when people switch backends, we want things to keep working as best they can ...

I agree with what you wrote which is why I opened #201 but in this instance this is a separate issue - this is about name clashes which we intend to avoid (and should be able to avoid!) no matter what backend is selected. This isn't quite the same problem as the fact that different backends accept different sets of keywords which is indeed the case we should generate warnings (the issue you describe). Earlier, I stated that we might need to raise an exception if the keyword is not recognized but I now realize this is unnecessary (you only need to check for clashes).

As this particular warning/exception is not supposed to ever happen (assuming the backends are implemented sensibly), simply raising an exception just seems easier to me. Again, if this condition does arise, it can be considered a bug in the backend implementation so it doesn't really matter whether you warn or raise an exception which is why I would just go for the simplest option.

@jbednar
Copy link
Member Author

jbednar commented Jul 25, 2015

You said:

We will also have to immediately raise an exception if the user tries to use this simplified syntax and mistypes a keyword name

What I'm arguing above is that this policy would prevent forward compatibility -- allowing a plot written for a newer version of HoloViews to come out basically ok in an older version, apart from the warning and whatever that option does not being respected. The vast majority of such options are, well, optional, such as setting line widths, fonts, specific colors, etc. Having some plot come out, with a warning, is much more reassuring to a user who is trying to get his colleague's notebook to work on his system than just seeing an exception and no output, unless he's specifically requested warnings as exceptions.

Note that I'm not arguing that I'm certain an exception would never be a good idea for an option setting, but that we should explicitly consider whether we would want to raise such an exception, as a policy, so that we decide on a rational balance of error detection versus issues of forward and backward compatibility.

I'm not sure if you're actually disagreeing with this now, given your latest comment...

@jbednar
Copy link
Member Author

jbednar commented Jul 25, 2015

Oh, and as for name clashes, can we really guarantee they will never happen? Don't we just pass style options down to the backend, without translation? Or would we translate such an option's name if it clashed with our own plot options?

@jlstevens
Copy link
Contributor

I'm not sure if you're actually disagreeing with this now, given your latest comment...

We aren't disagreeing - in my last comment, I corrected myself when I said we don't need to raise the exception that you quoted.

Don't we just pass style options down to the backend, without translation?

That is correct but we do know what those options are ahead of time.

@jbednar
Copy link
Member Author

jbednar commented Jul 25, 2015

We don't know which backends will be added in the future, but we could commit to forcing any clashes to be filtered/renamed...

@philippjfr philippjfr added this to the v1.10 milestone Dec 21, 2017
@philippjfr
Copy link
Member

I've come to the conclusion that I am strongly in favor of this suggestion, I don't know of any instances where plot and style options clash and where they do I think we could simply raise an error that the options couldn't be disambiguated. Having the user distinguish plot and style options is imo the main source of confusion when using HoloViews and also massively increases verbosity.

I've assigned this to v1.10 so we can pick this discussion up again.

@jlstevens
Copy link
Contributor

This would be a major API change so I think this is a version 2.0 feature request.

@jlstevens jlstevens modified the milestones: v1.10, v2.0 Dec 21, 2017
@philippjfr philippjfr modified the milestones: v2.0, v1.10 Dec 21, 2017
@philippjfr
Copy link
Member

This would be a major API change so I think this is a version 2.0 feature request.

I don't see how this would introduce any backward compatibility concerns so I'm moving back to 1.10.

@philippjfr
Copy link
Member

That doesn't mean it will get addressed for 1.10 of course, but I don't see any reason it couldn't be.

@jlstevens
Copy link
Contributor

One other option to consider is something like:

image.style(cmap='jet')

Ideally you would have .style and .plot leaving .opts to handl both when necessary. The problem with this is that .plot sounds like a command (which it is not) and .plotopts is awkward.

I think I would be happy with a .style method just for setting style options and allowing .opts to set plot options if you don't use the style and plot keywords (i.e in the style Jim suggested at the start of this issue).

I think this would make a lot of usage easier without discarding the distinction between these option types, which I believe is important to retain (e.g so users know where they are documented, when switching between plotting extensions etc). I know I would be happier with an approach like this than flattening the namespace.

@jbednar
Copy link
Member Author

jbednar commented Dec 22, 2017

I really don't see any reason to force users to remember the distinction between the two types. Our docs can keep them straight, which allows us to make the distinction when we need to explain things, but why should users have to remember it just to use them? Plus the verbosity is very irritating. Let's give our users a break!

@ea42gh
Copy link
Contributor

ea42gh commented Dec 22, 2017

How would you address setting opts for a particular backend with this suggestion?
An optional backend='matplotlib' argument?
Or rely on the current backend: I'd have different arguments for the backends the way I currently do?

@jlstevens
Copy link
Contributor

I really don't see any reason to force users to remember the distinction between the two types.

That is how they know whether to look in the holoviews docs, the bokeh docs or the matplotlib docs. I think it is a very important distinction.

@jbednar
Copy link
Member Author

jbednar commented Dec 22, 2017

Sure, it's a very important distinction for us as developers to make and for us to make clear in our docs; users do need to be able to find out which kind it is. But they really don't need to remember which kind it is to use it. E.g. "height" -- why should they care whether it's a plot or style option? People can use it just fine without consulting any documentation, and it's just an irritation to have to try one way and then switch it to the other to make it work. There's no deep reason it is a plot vs. a style option, at least not one evident to users, so why force everyone to remember arbitrary information like that, or to use trial and error to get it to work? It's a very unfriendly policy; we as developers can keep the two types straight all we want without having to require each and every user to do it every time they use them. Why make users jump through pointless hoops, and make all our examples and applications needlessly more verbose?

@philippjfr
Copy link
Member

Finally implemented.

Copy link

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
tag: API type: feature A major new feature
Projects
None yet
Development

No branches or pull requests

4 participants