Updated various notebooks to use opts #3315
Conversation
philippjfr
force-pushed
the
doc_updates
branch
from
252d028
to
a4933b0
Compare
| "sample_style = dict(edgecolors='k', alpha=1)\n", | ||
| "all_samples = obs_hmap.collapse().to.scatter3d().options(alpha=0.15, xticks=4)\n", | ||
| "sampled = obs_hmap.sample((3,3))\n", | ||
| "subsamples = sampled.to.scatter3d().options(**sample_style)\n", | ||
| "subsamples = sampled.to.scatter3d().opts(**sample_style)\n", |
There was a problem hiding this comment.
The dict should be made into keywords and should be split onto a separate line.
There was a problem hiding this comment.
They're reused at least twice.
There was a problem hiding this comment.
In that case make a single options object with an options builder and reuse that.
| "sampled = obs_hmap.sample((3,3), bounds=(2,5,5,10))\n", | ||
| "subsamples = sampled.to.scatter3d().options(**dict(sample_style, xticks=4))\n", | ||
| "subsamples = sampled.to.scatter3d().opts(**dict(sample_style, xticks=4))\n", |
There was a problem hiding this comment.
**dict seems unnecessary...
| " streams.Stream.define('Time', time=1.0)(),\n", | ||
| " streams.PointerX().rename(x='limit')]\n", | ||
| "\n", | ||
| "integral_dmap = hv.DynamicMap(integral, streams=integral_streams)\n", |
There was a problem hiding this comment.
Might be nice to call the dynamicmap integral and the callback something else...e.g integral_X or X_integral where X is descriptive..
| "%%opts RGB [width=300] {+axiswise}\n", | ||
| "\n", | ||
| "datashade(hv.HoloMap(gaussians, kdims='k')) + datashade(hv.HoloMap(lines, 'k'))" | ||
| "(datashade(hv.HoloMap(gaussians, kdims='k')) + datashade(hv.HoloMap(lines, 'k'))).opts(opts.RGB(framewise=True))" |
There was a problem hiding this comment.
Should be given a handle with .opts on another line.
| @@ -72,9 +72,9 @@ | |||
| " n = len(cms)*1.0\n", | |||
| " c=ceil(n/cols) if n>cols else cols\n", | |||
| " bars = [hv.Image(spacing, ydensity=1, label=\"{0} ({1})\".format(r.name,r.provider))\\\n", | |||
| " .options(cmap=process_cmap(r.name,provider=r.provider), **opts)\n", | |||
| " .opts(cmap=process_cmap(r.name,provider=r.provider), **opt_kwargs)\n", | |||
There was a problem hiding this comment.
I think I would prefer to inline those options instead of using **.
There was a problem hiding this comment.
Seems way too long in this case.
| "\n", | ||
| "np.set_printoptions(precision=2, linewidth=80)\n", | ||
| "hv.Dimension.type_formatters[np.float64] = '%.2f'\n", | ||
| "%opts HeatMap (cmap=\"hot\")" | ||
| "opts.defaults(opts.HeatMap(cmap='fire'), opts.Layout(shared_axes=False))" |
There was a problem hiding this comment.
opts.defaults should general be formatted like .opts with entries on newlines (when you have more than one)
There was a problem hiding this comment.
I don't think that's sensible for one or two keywords defaults.
| "hv.extension('bokeh')" | ||
| "hv.extension('bokeh')\n", | ||
| "\n", | ||
| "opts.defaults(opts.Path(padding=0.1), opts.Polygons(padding=0.1))" |
| "%%opts Graph [color_index='circle']\n", | ||
| "%%opts Graph (node_size=10 edge_line_width=1)\n", | ||
| "kwargs = dict(width=800, height=800, xaxis=None, yaxis=None)\n", | ||
| "opts.defaults(opts.Nodes(**kwargs), opts.Graph(**kwargs))\n", |
There was a problem hiding this comment.
Either the keywords should be inlined here, or in this case make explicit options objects with the builders (assuming there are lots of kwargs).
There was a problem hiding this comment.
Don't agree with inlining, this avoids duplication. Don't quite follow the second suggestion.
| @@ -186,9 +180,9 @@ | |||
| "metadata": {}, | |||
| "outputs": [], | |||
| "source": [ | |||
| "%%opts Image [xaxis='bare', yaxis='bare' show_title=False, colorbar=True] (cmap='fire')\n", | |||
| "%%output holomap='mp4'\n", | |||
There was a problem hiding this comment.
%%output can be removed.
|
@philippjfr Thanks for updating all these! I'm happy to see all the magics removed. Most of my comments are about following the guidelines we are recommending people use when using options. In the customization user guide we state that we are following these guidelines so my comments are trying to make sure that is true... |
|
Thanks for updating these guides and implementing the suggestions. There may always be some minor tweaks in subsequent PRs but the main thing is to get rid of the magics. Merging. |
User guides