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

Replacement for cell magic with tab-completion #3173

Merged
merged 35 commits into from
Nov 17, 2018
Merged

Conversation

jlstevens
Copy link
Contributor

@jlstevens jlstevens commented Nov 12, 2018

Work in progress to address #3095. I'm mostly happy with this approach, illustrated here:

image

Notes:

  • This is not an exact replacement for %%opts unless you use clone=False. The magics set options on the displayed object, not a clone of it.
  • This is still the line magic equivalent to %opts Curve (color='k'):
    opts("Curve (color='k')") # Or equivalently
    opts({'Curve': {'style': {'color':'k'}}})
  • This is still the more direct cell magic equivalent (without tab-completion) to
    %%opts Curve (color='k'):
    curve = hv.Curve([1,2,3])
    opts("Curve (color='k')", curve) # Or equivalently
    opts({'Curve': {'style': {'color':'k'}}}, curve)
  • opts can now tab-complete all possible options if you only use keywords, returning an Option object.
  • Option objects have been enhanced with improved validation and reprs (and can now take something like Curve.Group.Label as a key.
  • Switching the backend updates the completer objects (and the docstrings needed for tab-completion)
  • The opts entries do not exist until a backend is loaded.
  • There is a minor API change to .options as you can no longer set backend as a positional argument.

TODO:

  • Check set_current_backend is used correctly throughout.
  • Fix setting the opts class docstring so it isn't clobbered.
  • Move the merge option utility out of the parser.
  • Unit tests.
  • Post an ad hoc script to demonstrate automated conversion from cell magics. [Would be nice, doesn't have to be in this PR though]
  • Update docs and new docs [To be done in the next PR]

Decisions to make [We can address these in the docs update PR]:

  • Qualified imports always or can we make opts available in the namespace of examples? (I vote yes)
  • Default name for the list of Option objects (I think options isn't bad but a bit long)
  • Improve the API of Options now it is more user facing than before? i.e make some methods private? Should we document any of the methods/properties at all? Are they useful (e.g the cyclic predicate?)

@jlstevens
Copy link
Contributor Author

jlstevens commented Nov 13, 2018

Annoyingly, the signature options(self, *args, backend=None, clone=True, **kwargs) only works in Python 3. For Python 2 support I have to just use *args, **kwargs.

@jbednar
Copy link
Member

jbednar commented Nov 13, 2018

Looks great to me; thanks for seeing this through! Finally, we can have (nearly) everything we want!

@jlstevens
Copy link
Contributor Author

@philippjfr I've ticked off 'Check set_current_backend is used correctly throughout.' having checked that set_current_backend is used where I think it is necessary (see 34f2318, b857670).

I decided not to replace all the places where current_backend is set directly in the tests as that would result in a lot of relatively pointless changes: set_current_backend is only needed to run the hooks which the tests don't rely on (and the only hook is for building the completers on hv.opts).

@philippjfr
Copy link
Member

Sounds good.

@jlstevens
Copy link
Contributor Author

I decided I might as well add tab-completion to hv.output while I'm at it...

@philippjfr
Copy link
Member

Sounds good, although I've not used hv.output myself. Unless you object it would also be good to fix the error message when applying options when no backend has been imported, e.g. when running this:

import holoviews as hv
hv.Curve([1, 2, 3]).options(xaxis=None)

you currently get this useless error:

~/development/holoviews/holoviews/core/options.py in options(cls, backend, val)
   1057         backend = cls.current_backend if backend is None else backend
   1058         if val is None:
-> 1059             return cls._options[backend]
   1060         else:
   1061             cls._options[backend] = val

KeyError: 'matplotlib'

Would be nice to put an informative error message in Store.options if no backend has been imported.

@philippjfr
Copy link
Member

philippjfr commented Nov 15, 2018

Another thought I just had is that we are reasonably close to being able to validate the option values early. My op PR defines an interface to validate any style option for the bokeh backend using bokeh's property system like this:

from holoviews.plotting.bokeh.styles import validate
validate('line_color', 'red')

And once holoviz/param#300 is merged it'll be easy to do the same for plot options. I think that could be really useful, even if it isn't enabled by default (at least at first).

@jbednar
Copy link
Member

jbednar commented Nov 15, 2018

Presumably we can then also not just validate the option values, but also tab complete them and show them in the help?

@philippjfr
Copy link
Member

Presumably we can then also not just validate the option values, but also tab complete them and show them in the help?

That only works for certain options, so at least as a first cut this seems like too much complexity for not that much gain.

@jlstevens
Copy link
Contributor Author

@philippjfr I added a unit test in 79950f6. I'm not entirely happy with it but maybe it will do?

@jlstevens
Copy link
Contributor Author

@philippjfr I would like to see this PR merged so we can start testing it (and you could try op/dim with it). I've punted a few tasks to the docs PR that will follow but for now I think it is ready for review/merge (assuming the tests go green).

@philippjfr
Copy link
Member

Was hoping to see a few more unit tests for the new accepted .options formats, but I'll merge if you want to add those later.

@philippjfr
Copy link
Member

That's my only review comment really.

@jlstevens
Copy link
Contributor Author

That is a good idea, I'll add a few more unit tests along those lines.

@philippjfr philippjfr merged commit 1778be0 into master Nov 17, 2018
@philippjfr philippjfr added this to the v1.11.0 milestone Nov 17, 2018
@philippjfr philippjfr deleted the options_extension branch December 13, 2018 04:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants