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

Added .options method for simplified option setting #2306

Merged
merged 14 commits into from Feb 20, 2018

Conversation

Projects
None yet
3 participants
@philippjfr
Member

philippjfr commented Feb 5, 2018

Adds a .options method to all objects making it simpler to set options in a flattened way. Two formats are supported:

img.options(width=600, cmap='viridis')

and the more explicit:

img.options({'Image': dict(width=600, cmap='viridis')})

which allows usage on complex nested objects.

  • Addresses #222, #803, #2287
  • Add unit tests
  • Add docstrings
  • Add section in getting started and user guides
  • Move logic into separate utility
  • Add DynamicMap.options
@philippjfr

This comment has been minimized.

Member

philippjfr commented Feb 10, 2018

I'm very happy with this PR now, the .options method is really convenient and because it has to actually resolve the difference between different kinds of options its also much stricter than other approaches, which should result in a better user experience.

The main question I have now where to document it (I want a section in both the getting started and user guide) and what our guidance on using it is. Should we use it throughout the docs? @jbednar @jlstevens

@philippjfr

This comment has been minimized.

Member

philippjfr commented Feb 10, 2018

Ready to review.

@philippjfr philippjfr removed the WIP label Feb 10, 2018

@jbednar

This comment has been minimized.

Member

jbednar commented Feb 10, 2018

I think we should use this approach consistently throughout the docs, certainly in all places currently using .opts, but also replacing magics in many cases.

@philippjfr philippjfr requested a review from jlstevens Feb 13, 2018

@@ -105,6 +105,75 @@ def __call__(self, *args, **params):
return StoreOptions.set_options(obj, options)
@classmethod
def expand_options(cls, options, backend=None):

This comment has been minimized.

@jlstevens

jlstevens Feb 13, 2018

Member

If there are any well-defined units of code in this rather long method, I would like them pulled out as private helper classmethods. The warning generation bit can be split out this way I think and there might be other sensible units to pull out.

@@ -1326,13 +1326,13 @@ def tree_to_dict(cls, tree):
return specs
@classmethod
def propagate_ids(cls, obj, match_id, new_id, applied_keys):
def propagate_ids(cls, obj, match_id, new_id, applied_keys, backend=None):

This comment has been minimized.

@jlstevens

jlstevens Feb 13, 2018

Member

Glad to see this generalized to support the backend argument.

self.map(lambda x: setattr(x, 'id', None))
elif clone:
obj = self.map(lambda x: x.clone(id=x.id))
StoreOptions.set_options(obj, options, backend=backend, **kwargs)

This comment has been minimized.

@jlstevens

jlstevens Feb 13, 2018

Member

I think obj=self is really confusing here as it is not accessed until the line:

StoreOptions.set_options(obj, options, backend=backend, **kwargs)

Which can now be expressed in a way I find much more intuitive, removing the need for obj=self entirely:

StoreOptions.set_options(obj if clone else self, options, backend=backend, **kwargs)

This comment has been minimized.

@jlstevens

jlstevens Feb 13, 2018

Member

I'll also note that although support for the clone argument is reasonable, I would have preferred to see it in a separate PR.

This comment has been minimized.

@philippjfr

philippjfr Feb 13, 2018

Member

I think obj=self is really confusing here as it is not accessed until the line:

Not quite sure what you mean, it's overridden in the conditional and used twice. I'd have to do this instead:

StoreOptions.set_options(obj if clone else self, options, backend=backend, **kwargs)
return obj if clone else self
@jlstevens

This comment has been minimized.

Member

jlstevens commented Feb 13, 2018

I haven't looked at the changes to the docs (i.e notebooks) just yet but I have looked at the code and made a few comments. It mostly looks fine and I am happy the bulk of the new functionality is implemented as a classmethod on hv.opts.

if backend is None:
# Check option is invalid for all backends
found = []
for lb in [b for b in loaded_backends if b != 'backend']:

This comment has been minimized.

@jlstevens

jlstevens Feb 13, 2018

Member

We spotted this bug together - it should be backend (a variable) not 'backend' a string. Is there a unit test that would have caught this bug? The tests did go green....

This comment has been minimized.

@philippjfr

philippjfr Feb 13, 2018

Member

Just went through this, it's not tested because this is just an optimization to ensure it doesn't check the same backend twice.

@philippjfr

This comment has been minimized.

Member

philippjfr commented Feb 18, 2018

Ready to review again.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Feb 19, 2018

Happy to review once the merge conflicts are resolved. I doubt I'll have many new comments to add once that is done.

"""
Generates an error message for an invalid option suggesting
similar options through fuzzy matching.
"""

This comment has been minimized.

@jlstevens

jlstevens Feb 19, 2018

Member

I should have raised this earlier. Isn't all this logic what OptionError is for? The logic is certainly closely related so maybe OptionError should be enhanced or extended somehow...

This comment has been minimized.

@philippjfr

philippjfr Feb 19, 2018

Member

I did look into that but I can be much more eager about the validation here because the user can declare an explicit backend and because it has to resolve each option into the appropriate group. This also means I can supply a more specific error message. I think we should look at unifying the validation, especially once the magic allows declaring an explicit backend.

What I would like to know is why the OptionError handling is so horrendously complex. Why does it have to record errors? It seems like that code would be hugely simplified by a utility that checks whether an option is invalid across all backends, so you can eagerly raise as soon as you encounter an unsupported option rather than processing all the options and checking validity at the end and having 3 methods to record options, a context manager to control the option validation, a bunch of state on the OptsMagic etc.

Separately I'm still confused in what cases the existing code actually validates anything, it seems like .opts still has zero validation, since this raises no error:

hv.Curve([]).opts(plot=dict(some_nonexistent_option=3))

This comment has been minimized.

@jlstevens

jlstevens Feb 19, 2018

Member

What I would like to know is why the OptionError handling is so horrendously complex. Why does it have to record errors? It seems like that code would be hugely simplified by a utility that checks whether an option is invalid across all backends, so you can eagerly raise as soon as you encounter an unsupported option rather than processing all the options and checking validity at the end.

The main reason it is this way is because the errors should be derived from the allowed_keywords specified in the Options. It is true that these allowed keywords are set from the plot and style option declarations which is what your proposed utility would rely on directly. I'm certainly not opposed to simplifying the code with a more direct check but the current design allows more flexibility by having a level of indirection: plot and style option declarations -> allowed keywords on options and errors-> validation.

As I agree we rarely use the extra flexibility that this design affords us, I would be happy to see a PR that simplifies things to plot and style option declarations -> validation. My worry is that such a PR might have to touch a lot more than you might expect (e.g how the errors are processed and printed by the %opts magic(s)).

This comment has been minimized.

@philippjfr

philippjfr Feb 19, 2018

Member

Right, I definitely want to tackle and simplify that code because I'm having a really tough time following it and I think I'll much prefer an approach that eagerly errors when it hits an unsupported option. But as you say it touches a lot of hairy code so I think I'd like to do that separately.

Currently I'm leaning toward merging this PR and getting a dev release out and then to follow up with a PR to try to unify the option validation. What do you think?

This comment has been minimized.

@jlstevens

jlstevens Feb 19, 2018

Member

I'll much prefer an approach that eagerly errors when it hits an unsupported option.

That is exactly how it worked when we only ever had to worry about a single backend at a time. The complexity was introduced when we decided to merge the allowed keywords across backends which was implemented in such a manner as to extend the old system (e.g where the magics used the OptionError message to generate a pretty printed HTML suggestion).

Currently I'm leaning toward merging this PR and getting a dev release out and then to follow up with a PR to try to unify the option validation. What do you think?

Makes sense. I don't think of it as 'unifying' the option validation though (it is already unified, which is what makes it ugly). That PR would be more of a refactor/redesign by validating without looking at the allowed keywords for Options.

I suppose one thing the current design lets you do is to declare options for elements/plots that don't exist, using allowed_keywords to do validation. The current design keeps the option system decoupled from elements/plots with allow_keywords acting as the link between them (set somewhere on Store iirc). This (in theory) could let you use the option system in a completely different project/library.

In practice, this decoupling doesn't seem to do much useful (unless you can imagine using the options system for something without needing a corresponding element/plot). In other words, I think there is a sound design underlying the way it is right now but 1) it doesn't add much of value in practice 2) it is becomes complicated once you start taking the union between backends. For this reason, I would be happy to see things simplified, even if it means coupling the options system closer to the elements and plot classes.

This comment has been minimized.

@philippjfr

philippjfr Feb 19, 2018

Member

Don't think we necessarily need to couple the two closer together (although I haven't considered all the implications). I just feel like it comes down to the way you iterate over the backends. Currently it seems like looping over the backends is the outer loop, which means you always iterate over all backends and can't raise until you've processed them all. The approach I've taken is to validate the current (or explicitly selected) backend first, and if an option is invalid for that backend it falls back to checking whether the option is valid for other backends. This means that as soon as an invalid option is encountered it can raise an error (or warning) rather than having to process everything first and only asserting whether an option was invalid at the end.

My only question then is about the OptsMagic, is there any reason why that can't raise eagerly or does it somehow have to catch errors and reraise them in a special way?

This comment has been minimized.

@jlstevens

jlstevens Feb 19, 2018

Member

does it somehow have to catch errors and reraise them in a special way?

That is exactly what it does.

This comment has been minimized.

@philippjfr

philippjfr Feb 19, 2018

Member

Okay, I can still do that easily enough.

This comment has been minimized.

@jlstevens

jlstevens Feb 19, 2018

Member

Though I will note it does it using the validation_error_message method which might mean there is indeed one single place where your approach could fit in nicely. Of course, there may still be other consequences I haven't thought about...e.g how will normal error validation work if you work with option trees directly?

This comment has been minimized.

@philippjfr

philippjfr Feb 19, 2018

Member

When you work with an OptionTree directly the allowed_keywords are well defined and no cross-backend validation is needed so that should be fine. I'll open a PR branching off this one to see what it could look like.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Feb 19, 2018

Other than our ongoing discussion (which will become another PR), I am happy to merge once all the tests are green (though I think another PR that is about to be merged will touch the test data).

@philippjfr

This comment has been minimized.

Member

philippjfr commented Feb 19, 2018

Making good progress on getting the validation unified so +1 on merging asap. This PR shouldn't touch test data so I'll try to get it passing without waiting on the other PR.

@philippjfr

This comment has been minimized.

Member

philippjfr commented Feb 19, 2018

Also finding a number of bugs in the existing option validation, so refactoring this is definitely a good idea. How do you feel about dropping the skip_invalid option? I feel like it should either warn or error, although I'm happy to be persuaded otherwise until we have a way of specifying an explicit backend in the opts magic.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Feb 19, 2018

How do you feel about dropping the skip_invalid option?

Fine by me. If I remember correctly, that was the first approach we had when we started supporting multiple backends. I do think it is less relevant now.

@philippjfr philippjfr referenced this pull request Feb 19, 2018

Open

Option error refactor #2354

0 of 2 tasks complete
@philippjfr

This comment has been minimized.

Member

philippjfr commented Feb 19, 2018

Ready to merge now, refactoring will happen in #2354.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Feb 20, 2018

As the tests are passing, I'll go ahead and merge.

@jlstevens jlstevens merged commit 76c7cc8 into master Feb 20, 2018

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.02%) to 82.081%
Details
s3-reference-data-cache Tests passing no test data changes required.
Details

@philippjfr philippjfr added this to the v1.10 milestone Feb 20, 2018

@philippjfr philippjfr deleted the simplified_option_setting branch Feb 20, 2018

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