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

Conversation

Projects
None yet
3 participants
@jlstevens
Copy link
Contributor

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor

jbednar commented Nov 13, 2018

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

@jlstevens

This comment has been minimized.

Copy link
Contributor Author

jlstevens commented Nov 13, 2018

@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

This comment has been minimized.

Copy link
Contributor

philippjfr commented Nov 14, 2018

Sounds good.

@jlstevens

This comment has been minimized.

Copy link
Contributor Author

jlstevens commented Nov 14, 2018

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

@philippjfr

This comment has been minimized.

Copy link
Contributor

philippjfr commented Nov 14, 2018

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

This comment has been minimized.

Copy link
Contributor

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 pyviz/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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor

philippjfr 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?

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

This comment has been minimized.

Copy link
Contributor Author

jlstevens commented Nov 15, 2018

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

@jlstevens

This comment has been minimized.

Copy link
Contributor Author

jlstevens commented Nov 15, 2018

@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

This comment has been minimized.

Copy link
Contributor

philippjfr commented Nov 16, 2018

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

This comment has been minimized.

Copy link
Contributor

philippjfr commented Nov 16, 2018

That's my only review comment really.

@jlstevens

This comment has been minimized.

Copy link
Contributor Author

jlstevens commented Nov 16, 2018

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

jlstevens added some commits Nov 16, 2018

@philippjfr philippjfr merged commit 1778be0 into master Nov 17, 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 decreased (-0.003%) to 89.092%
Details
s3-reference-data-cache Test data is cached.
Details

@philippjfr philippjfr added this to the v1.11.0 milestone Nov 17, 2018

@philippjfr philippjfr deleted the options_extension branch Dec 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.