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

Modernize notebook: opensky #396

Merged
merged 11 commits into from
Nov 21, 2024
Merged

Modernize notebook: opensky #396

merged 11 commits into from
Nov 21, 2024

Conversation

Azaya89
Copy link
Collaborator

@Azaya89 Azaya89 commented May 31, 2024

Modernizing an example checklist

Preliminary checks

  • Look for open PRs and issues that reference the project you are updating. It is possible previous unmerged work in PR could be re-used to modernize the project. Comment on these PRs and issues when appropriate, hopefully we should be able to close some of them after your modernizing work.

Change ‘anaconda-project.yml’ to use the latest workable version of packages

  • Pin python=3.11
  • Remove the upper pin (e.g. hvplot<0.9 to hvplot, panel>=0.12,<1.0 to panel>=0.12) of all other dependencies. Removing the upper pins of dependencies could necessitate code revisions in the notebooks to address any errors encountered in the updated environment. Should complexities or extensive time requirements arise, document issues for team discussion on whether to re-pin specific packages or explore other solutions.
  • Add/update the lower pin of all other dependencies (e.g. hvplot to hvplot>=0.9.2, hvplot>=0.8 to hvplot>=0.9.2). Usually, the new/updated lower pin of a dependency will be the version resolved after anaconda prepare has been run. Execute !conda list in a notebook, or anaconda run conda list in the terminal, to display the version of each dependency installed in the environment. Adjusting the lower pin helps ensure that the locks produced for each platform (linux-64, win-64, osx-64, osx-arm64) rely on the tested dependencies and not on some older versions.
  • If one of the channels include conda-forge or pyviz, ask Maxime if it can be removed

Plot API updates (discussed on a per-example basis)

  • Generally, try to replace HoloViews usage with hvPlot. At a certain point of complexity, such as with the use of ‘.select’, it might be better to stick with HoloViews. Additional examples of ‘complexity boundaries’ should be documented in this document.
  • Almost always, try to replace the use of datashade with rasterize (read this page). Essentially, rasterize allows Bokeh to handle the colormapping instead of Datashader.

Interactivity API updates (discussed on a per-example basis)

  • Remove all pn.interact usage
  • Avoid .param.watch() usage. This is pretty low-level and verbose approach and should not be used in Examples unless required, or an Example is specifically trying to demo its usage in an advanced workflow.
  • Prefer using pn.bind(). Read this page for explanation.
  • For apps built using a class approach, when they create a view() method and call it directly, update the class by inheriting from pn.viewable.Viewer and replace view() by __panel__(). Here is an example.

Panel App updates (discussed on a per-example basis)

  • If the project doesn’t at any point create a Panel app at all, consider creating one. It can be as simple as wrapping a plot in pn.Column, or more complicated to incorporate widgets, etc. Make the final app .servable().
  • If the project creates an app in a notebook but doesn’t deploy it (i.e. there is no command: dashboard declaration in the anaconda-project.yml file), try adding it.
  • If the project already deploys an app but doesn’t wrap it in a nice template, consider wrapping it in a template.
  • If the project deploys an app wrapped in a template, customize the template a little so all the apps don’t look similar (e.g. change the header background color). This doesn’t need to be discussed.
  • Comment start If you are building the application in a single cell, you can construct a template explicitly, like template = pn.template.BootstrampTemplate, but if building up an app across multiple cells, it is probably cleaner to declare the template at the top with pn.extension(template='bootstrap'). See how to guide on setting a template.

General code quality updates

  • If the notebook disables warnings (e.g. with warnings.simplefilter(‘ignore’) somewhere at the start of the notebook, remove this line. Try to update the code to remove the warnings, if any. If updating the code to remove the warnings is taking significant amount of time and effort, bring it up for discussion and we may decide to disable warnings again.

Text content

  • Edit the text content anywhere and everywhere that it can be improved for clarity.
  • Check the links are valid, and update old links (e.g. http -> https, xyz.pyviz.org -> xyz.holoviz.org)
  • Remove instructions to install packages inside an example

Visual appearance - Example

  • Check that the titles/headings make sense and are succinct.
  • Check that the text content blocks are easily readable; revise into additional paragraphs if needed.
  • Check that the code blocks are easily readable; revise as needed. (e.g. add spaces after commas in a list if there are none, wrap long lines, etc.)
  • Check image and plot sizes. If possible, making them responsive is highly recommended.
  • Check the appearance on a smartphone (check Google to see how to adapt the appearance of your browser to display pages as if they were seen from a smartphone, this is usually done via the web developer tools). This is not a top priority for all examples, but if there are a few easy and straightforward changes to make that can improve the experience, let’s do it.
  • Check the updated notebook with the original notebook

Visual appearance - Gallery

  • Check the thumbnail is visually appealing
  • Check the project title is well formatted (e.g. Ml Annotators to ML Annotators), if not, add/update the examples_config.title field in anaconda-project.yml
  • Check the project description is appropriate, if not, update the description field in anaconda-project.yml

Workflow (after you have made the changes above)

  • Run successfully doit validate:<projectname>
  • Run successfully doit test:<projectname>
  • Run successfully doit doc_one –name <projectname>. It’s better if the project notebook(s) is saved with its outputs (but be sure to clear outputs before committing to the examples repo!) when building the docs. Then open this file in your browser ./builtdocs/index.html and check how the site looks.
  • If you’re happy with all the above, open a PR. Reminder, clear notebook outputs before pushing to the PR.

@Azaya89 Azaya89 added the NF SDG NumFocus Software Development Grant 2024 label May 31, 2024
@Azaya89 Azaya89 self-assigned this May 31, 2024
@Azaya89 Azaya89 requested a review from maximlt May 31, 2024 14:41
Copy link
Contributor

Your changes were successfully integrated in the dev site, make sure to review
the pages of the projects you touched before merging this PR: https://holoviz-dev.github.io/examples/.
You can also download an archive of the site from the workflow summary page which comes in handy
when your dev site built was overriden by another PR (we have a single dev site!).

1 similar comment
Copy link
Contributor

github-actions bot commented Jun 4, 2024

Your changes were successfully integrated in the dev site, make sure to review
the pages of the projects you touched before merging this PR: https://holoviz-dev.github.io/examples/.
You can also download an archive of the site from the workflow summary page which comes in handy
when your dev site built was overriden by another PR (we have a single dev site!).

@maximlt
Copy link
Contributor

maximlt commented Jun 5, 2024

Hey @Azaya89, you ticked the first item in the checklist. Did you see this issue #112? If not, please comment on it.

- notebook<7
- bokeh>=3.4.0
- colorcet>=3.1.0
- dask>=2023.11
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is Dask used in this example?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, but adding the latest version of Dask silences some future warnings that were popping up.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we don't use Dask at all, let's just remove it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still to be done as far as I can see.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

opensky/anaconda-project.yml Outdated Show resolved Hide resolved
Copy link
Contributor

github-actions bot commented Jun 7, 2024

Your changes were successfully integrated in the dev site, make sure to review
the pages of the projects you touched before merging this PR: https://holoviz-dev.github.io/examples/.
You can also download an archive of the site from the workflow summary page which comes in handy
when your dev site built was overriden by another PR (we have a single dev site!).

@Azaya89 Azaya89 requested a review from maximlt June 7, 2024 17:57
@@ -15,8 +15,6 @@
"\n",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't look like the "## OpenSky flight trajectories is useful since there's only one heading at this level. Alternatively, you could add more headings down the example if it makes sense to segment it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll just remove it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still to be done as far as I can see.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -15,8 +15,6 @@
"\n",
"Flight path information for commercial flights is available for some regions of the USA and Europe from the crowd-sourced [OpenSky Network](https://opensky-network.org/). OpenSky collects data from a large number of users monitoring public air-traffic control information. Here we will use a subset of the data that was polled from their REST API at an interval of 1 minute over 4 days (September 5-13, 2016), using the collect_data.py and prepare_data.py. In general the terms of use for OpenSky data do not allow redistribution, but we have obtained specific permission for distributing the subset of the data used in this project, which is a 200MB Parquet file (1.1GB as the original database). If you want more or different data, you can run the scripts yourself, or else you can contact OpenSky asking for a copy of the dataset.\n",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"Flight path information for commercial flights is available for some regions of the USA and Europe from the crowd-sourced [OpenSky Network](https://opensky-network.org/). OpenSky collects data from a large number of users monitoring public air-traffic control information. Here we will use a subset of the data that was polled from their REST API at an interval of 1 minute over 4 days (September 5-13, 2016), using the collect_data.py and prepare_data.py. In general the terms of use for OpenSky data do not allow redistribution, but we have obtained specific permission for distributing the subset of the data used in this project, which is a 200MB Parquet file (1.1GB as the original database). If you want more or different data, you can run the scripts yourself, or else you can contact OpenSky asking for a copy of the dataset.\n",
"Flight path information for commercial flights is available for some regions of the USA and Europe from the crowd-sourced [OpenSky Network](https://opensky-network.org/). OpenSky collects data from a large number of users monitoring public air-traffic control information. Here we will use a subset of the data that was polled from their REST API at an interval of 1 minute over 4 days (September 5-13, 2016), using the `collect_data.py` and `prepare_data.py` scripts. In general the terms of use for OpenSky data do not allow redistribution, but we have obtained specific permission for distributing the subset of the data used in this project, which is a 200MB Parquet file (1.1GB as the original database). If you want more or different data, you can run the scripts yourself, or else you can contact OpenSky asking for a copy of the dataset.\n",

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still to be done as far as I can see.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

" x_range=x_range, y_range=y_range, aggregator=ds.count())\n",
"hv.Tiles(tile_url) * datashaded"
"flightpaths.hvplot.paths('longitude', 'latitude', tiles='EsriStreet',\n",
" aggregator=ds.count(), datashade=True)"
]
},
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you remove (e.g. on Anaconda Cloud) from the paragraph below?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still to be done as far as I can see.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

"outputs": [],
"source": [
"aggregator = ds.by('origin')\n",
"flightpaths.hvplot.paths('longitude', 'latitude', tiles='EsriStreet', datashade=True,\n",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This plot looks quite different from the one before, and I think it doesn't look so good now.

Now:
image

Before:
image

Copy link
Contributor

@maximlt maximlt Jul 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked a bit more into that. In particular, you can see that in the 1st plot I shared above, there are paths going to the left edge of the x_range while they aren't there in the 2nd plot.

When running this code with the new environment, I get this errorValueError: Dimension name cannot be empty.

aggregator = ds.count_cat('origin')
datashaded = hd.datashade(hv.Path(flightpaths, ['longitude', 'latitude']), 
                          aggregator=aggregator, 
                          color_key=categorical_color_key(aggregator, 'hsv_r'))
datashaded.opts(width=850, height=600)
Traceback

WARNING:param.dynamic_operation: Callable raised "ValueError('Dimension name cannot be empty')".
Invoked as dynamic_operation(height=400, scale=1.0, width=400, x_range=None, y_range=None)
WARNING:param.dynamic_operation: Callable raised "ValueError('Dimension name cannot be empty')".
Invoked as dynamic_operation(height=400, scale=1.0, width=400, x_range=None, y_range=None)

---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
File ~/dev/examples/opensky/envs/default/lib/python3.11/site-packages/IPython/core/formatters.py:974, in MimeBundleFormatter.__call__(self, obj, include, exclude)
    971     method = get_real_method(obj, self.print_method)
    973     if method is not None:
--> 974         return method(include=include, exclude=exclude)
    975     return None
    976 else:

File ~/dev/examples/opensky/envs/default/lib/python3.11/site-packages/holoviews/core/dimension.py:1286, in Dimensioned._repr_mimebundle_(self, include, exclude)
   1279 def _repr_mimebundle_(self, include=None, exclude=None):
   1280     """
   1281     Resolves the class hierarchy for the class rendering the
   1282     object using any display hooks registered on Store.display
   1283     hooks.  The output of all registered display_hooks is then
   1284     combined and returned.
   1285     """
-> 1286     return Store.render(self)

File ~/dev/examples/opensky/envs/default/lib/python3.11/site-packages/holoviews/core/options.py:1428, in Store.render(cls, obj)
   1426 data, metadata = {}, {}
   1427 for hook in hooks:
-> 1428     ret = hook(obj)
   1429     if ret is None:
   1430         continue

File ~/dev/examples/opensky/envs/default/lib/python3.11/site-packages/holoviews/ipython/display_hooks.py:287, in pprint_display(obj)
    285 if not ip.display_formatter.formatters['text/plain'].pprint:
    286     return None
--> 287 return display(obj, raw_output=True)

File ~/dev/examples/opensky/envs/default/lib/python3.11/site-packages/holoviews/ipython/display_hooks.py:261, in display(obj, raw_output, **kwargs)
    259 elif isinstance(obj, (HoloMap, DynamicMap)):
    260     with option_state(obj):
--> 261         output = map_display(obj)
    262 elif isinstance(obj, Plot):
    263     output = render(obj)

File ~/dev/examples/opensky/envs/default/lib/python3.11/site-packages/holoviews/ipython/display_hooks.py:149, in display_hook.<locals>.wrapped(element)
    147 try:
    148     max_frames = OutputSettings.options['max_frames']
--> 149     mimebundle = fn(element, max_frames=max_frames)
    150     if mimebundle is None:
    151         return {}, {}

File ~/dev/examples/opensky/envs/default/lib/python3.11/site-packages/holoviews/ipython/display_hooks.py:209, in map_display(vmap, max_frames)
    206     max_frame_warning(max_frames)
    207     return None
--> 209 return render(vmap)

File ~/dev/examples/opensky/envs/default/lib/python3.11/site-packages/holoviews/ipython/display_hooks.py:76, in render(obj, **kwargs)
     73 if renderer.fig == 'pdf':
     74     renderer = renderer.instance(fig='png')
---> 76 return renderer.components(obj, **kwargs)

File ~/dev/examples/opensky/envs/default/lib/python3.11/site-packages/holoviews/plotting/renderer.py:396, in Renderer.components(self, obj, fmt, comm, **kwargs)
    393 embed = (not (dynamic or streams or self.widget_mode == 'live') or config.embed)
    395 if embed or config.comms == 'default':
--> 396     return self._render_panel(plot, embed, comm)
    397 return self._render_ipywidget(plot)

File ~/dev/examples/opensky/envs/default/lib/python3.11/site-packages/holoviews/plotting/renderer.py:403, in Renderer._render_panel(self, plot, embed, comm)
    401 doc = Document()
    402 with config.set(embed=embed):
--> 403     model = plot.layout._render_model(doc, comm)
    404 if embed:
    405     return render_model(model, comm)

File ~/dev/examples/opensky/envs/default/lib/python3.11/site-packages/panel/viewable.py:736, in Viewable._render_model(self, doc, comm)
    734 if comm is None:
    735     comm = state._comm_manager.get_server_comm()
--> 736 model = self.get_root(doc, comm)
    738 if self._design and self._design.theme.bokeh_theme:
    739     doc.theme = self._design.theme.bokeh_theme

File ~/dev/examples/opensky/envs/default/lib/python3.11/site-packages/panel/layout/base.py:320, in Panel.get_root(self, doc, comm, preprocess)
    316 def get_root(
    317     self, doc: Optional[Document] = None, comm: Optional[Comm] = None,
    318     preprocess: bool = True
    319 ) -> Model:
--> 320     root = super().get_root(doc, comm, preprocess)
    321     # ALERT: Find a better way to handle this
    322     if hasattr(root, 'styles') and 'overflow-x' in root.styles:

File ~/dev/examples/opensky/envs/default/lib/python3.11/site-packages/panel/viewable.py:667, in Renderable.get_root(self, doc, comm, preprocess)
    665 wrapper = self._design._wrapper(self)
    666 if wrapper is self:
--> 667     root = self._get_model(doc, comm=comm)
    668     if preprocess:
    669         self._preprocess(root)

File ~/dev/examples/opensky/envs/default/lib/python3.11/site-packages/panel/layout/base.py:186, in Panel._get_model(self, doc, root, parent, comm)
    184 root = root or model
    185 self._models[root.ref['id']] = (model, parent)
--> 186 objects, _ = self._get_objects(model, [], doc, root, comm)
    187 props = self._get_properties(doc)
    188 props[self._property_mapping['objects']] = objects

File ~/dev/examples/opensky/envs/default/lib/python3.11/site-packages/panel/layout/base.py:168, in Panel._get_objects(self, model, old_objects, doc, root, comm)
    166 else:
    167     try:
--> 168         child = pane._get_model(doc, root, model, comm)
    169     except RerenderError as e:
    170         if e.layout is not None and e.layout is not self:

File ~/dev/examples/opensky/envs/default/lib/python3.11/site-packages/panel/pane/holoviews.py:429, in HoloViews._get_model(self, doc, root, parent, comm)
    427     plot = self.object
    428 else:
--> 429     plot = self._render(doc, comm, root)
    431 plot.pane = self
    432 backend = plot.renderer.backend

File ~/dev/examples/opensky/envs/default/lib/python3.11/site-packages/panel/pane/holoviews.py:525, in HoloViews._render(self, doc, comm, root)
    522     if comm:
    523         kwargs['comm'] = comm
--> 525 return renderer.get_plot(self.object, **kwargs)

File ~/dev/examples/opensky/envs/default/lib/python3.11/site-packages/holoviews/plotting/bokeh/renderer.py:68, in BokehRenderer.get_plot(self_or_cls, obj, doc, renderer, **kwargs)
     61 @bothmethod
     62 def get_plot(self_or_cls, obj, doc=None, renderer=None, **kwargs):
     63     """
     64     Given a HoloViews Viewable return a corresponding plot instance.
     65     Allows supplying a document attach the plot to, useful when
     66     combining the bokeh model with another plot.
     67     """
---> 68     plot = super().get_plot(obj, doc, renderer, **kwargs)
     69     if plot.document is None:
     70         plot.document = Document() if self_or_cls.notebook_context else curdoc()

File ~/dev/examples/opensky/envs/default/lib/python3.11/site-packages/holoviews/plotting/renderer.py:217, in Renderer.get_plot(self_or_cls, obj, doc, renderer, comm, **kwargs)
    214     raise SkipRendering(msg.format(dims=dims))
    216 # Initialize DynamicMaps with first data item
--> 217 initialize_dynamic(obj)
    219 if not renderer:
    220     renderer = self_or_cls

File ~/dev/examples/opensky/envs/default/lib/python3.11/site-packages/holoviews/plotting/util.py:270, in initialize_dynamic(obj)
    268     continue
    269 if not len(dmap):
--> 270     dmap[dmap._initial_key()]

File ~/dev/examples/opensky/envs/default/lib/python3.11/site-packages/holoviews/core/spaces.py:1217, in DynamicMap.__getitem__(self, key)
   1215 # Not a cross product and nothing cached so compute element.
   1216 if cache is not None: return cache
-> 1217 val = self._execute_callback(*tuple_key)
   1218 if data_slice:
   1219     val = self._dataslice(val, data_slice)

File ~/dev/examples/opensky/envs/default/lib/python3.11/site-packages/holoviews/core/spaces.py:984, in DynamicMap._execute_callback(self, *args)
    981     kwargs['_memoization_hash_'] = hash_items
    983 with dynamicmap_memoization(self.callback, self.streams):
--> 984     retval = self.callback(*args, **kwargs)
    985 return self._style(retval)

File ~/dev/examples/opensky/envs/default/lib/python3.11/site-packages/holoviews/core/spaces.py:582, in Callable.__call__(self, *args, **kwargs)
    579     args, kwargs = (), dict(pos_kwargs, **kwargs)
    581 try:
--> 582     ret = self.callable(*args, **kwargs)
    583 except KeyError:
    584     # KeyError is caught separately because it is used to signal
    585     # invalid keys on DynamicMap and should not warn
    586     raise

File ~/dev/examples/opensky/envs/default/lib/python3.11/site-packages/holoviews/util/__init__.py:1037, in Dynamic._dynamic_operation.<locals>.dynamic_operation(*key, **kwargs)
   1036 def dynamic_operation(*key, **kwargs):
-> 1037     key, obj = resolve(key, kwargs)
   1038     return apply(obj, *key, **kwargs)

File ~/dev/examples/opensky/envs/default/lib/python3.11/site-packages/holoviews/util/__init__.py:1026, in Dynamic._dynamic_operation.<locals>.resolve(key, kwargs)
   1024 elif isinstance(map_obj, DynamicMap) and map_obj._posarg_keys and not key:
   1025     key = tuple(kwargs[k] for k in map_obj._posarg_keys)
-> 1026 return key, map_obj[key]

File ~/dev/examples/opensky/envs/default/lib/python3.11/site-packages/holoviews/core/spaces.py:1217, in DynamicMap.__getitem__(self, key)
   1215 # Not a cross product and nothing cached so compute element.
   1216 if cache is not None: return cache
-> 1217 val = self._execute_callback(*tuple_key)
   1218 if data_slice:
   1219     val = self._dataslice(val, data_slice)

File ~/dev/examples/opensky/envs/default/lib/python3.11/site-packages/holoviews/core/spaces.py:984, in DynamicMap._execute_callback(self, *args)
    981     kwargs['_memoization_hash_'] = hash_items
    983 with dynamicmap_memoization(self.callback, self.streams):
--> 984     retval = self.callback(*args, **kwargs)
    985 return self._style(retval)

File ~/dev/examples/opensky/envs/default/lib/python3.11/site-packages/holoviews/core/spaces.py:582, in Callable.__call__(self, *args, **kwargs)
    579     args, kwargs = (), dict(pos_kwargs, **kwargs)
    581 try:
--> 582     ret = self.callable(*args, **kwargs)
    583 except KeyError:
    584     # KeyError is caught separately because it is used to signal
    585     # invalid keys on DynamicMap and should not warn
    586     raise

File ~/dev/examples/opensky/envs/default/lib/python3.11/site-packages/holoviews/util/__init__.py:1038, in Dynamic._dynamic_operation.<locals>.dynamic_operation(*key, **kwargs)
   1036 def dynamic_operation(*key, **kwargs):
   1037     key, obj = resolve(key, kwargs)
-> 1038     return apply(obj, *key, **kwargs)

File ~/dev/examples/opensky/envs/default/lib/python3.11/site-packages/holoviews/util/__init__.py:1030, in Dynamic._dynamic_operation.<locals>.apply(element, *key, **kwargs)
   1028 def apply(element, *key, **kwargs):
   1029     kwargs = dict(util.resolve_dependent_kwargs(self.p.kwargs), **kwargs)
-> 1030     processed = self._process(element, key, kwargs)
   1031     if (self.p.link_dataset and isinstance(element, Dataset) and
   1032         isinstance(processed, Dataset) and processed._dataset is None):
   1033         processed._dataset = element.dataset

File ~/dev/examples/opensky/envs/default/lib/python3.11/site-packages/holoviews/util/__init__.py:1012, in Dynamic._process(self, element, key, kwargs)
   1010 elif isinstance(self.p.operation, Operation):
   1011     kwargs = {k: v for k, v in kwargs.items() if k in self.p.operation.param}
-> 1012     return self.p.operation.process_element(element, key, **kwargs)
   1013 else:
   1014     return self.p.operation(element, **kwargs)

File ~/dev/examples/opensky/envs/default/lib/python3.11/site-packages/holoviews/core/operation.py:194, in Operation.process_element(self, element, key, **params)
    191 else:
    192     self.p = param.ParamOverrides(self, params,
    193                                   allow_extra_keywords=self._allow_extra_keywords)
--> 194 return self._apply(element, key)

File ~/dev/examples/opensky/envs/default/lib/python3.11/site-packages/holoviews/core/operation.py:141, in Operation._apply(self, element, key)
    139     if not in_method:
    140         element._in_method = True
--> 141 ret = self._process(element, key)
    142 if hasattr(element, '_in_method') and not in_method:
    143     element._in_method = in_method

File ~/dev/examples/opensky/envs/default/lib/python3.11/site-packages/holoviews/operation/datashader.py:1527, in datashade._process(self, element, key)
   1526 def _process(self, element, key=None):
-> 1527     agg = rasterize._process(self, element, key)
   1528     shaded = shade._process(self, agg, key)
   1529     return shaded

File ~/dev/examples/opensky/envs/default/lib/python3.11/site-packages/holoviews/operation/datashader.py:1506, in rasterize._process(self, element, key)
   1503     op = transform.instance(**{k:v for k,v in extended_kws.items()
   1504                                if k in transform.param})
   1505     op._precomputed = self._precomputed
-> 1506     element = element.map(op, predicate)
   1507     self._precomputed = op._precomputed
   1509 unused_params = list(all_supplied_kws - all_allowed_kws)

File ~/dev/examples/opensky/envs/default/lib/python3.11/site-packages/holoviews/core/data/__init__.py:196, in PipelineMeta.pipelined.<locals>.pipelined_fn(*args, **kwargs)
    193     inst._in_method = True
    195 try:
--> 196     result = method_fn(*args, **kwargs)
    197     if PipelineMeta.disable:
    198         return result

File ~/dev/examples/opensky/envs/default/lib/python3.11/site-packages/holoviews/core/data/__init__.py:1213, in Dataset.map(self, *args, **kwargs)
   1211 @wraps(LabelledData.map)
   1212 def map(self, *args, **kwargs):
-> 1213     return super().map(*args, **kwargs)

File ~/dev/examples/opensky/envs/default/lib/python3.11/site-packages/holoviews/core/dimension.py:697, in LabelledData.map(self, map_fn, specs, clone)
    695     return deep_mapped
    696 else:
--> 697     return map_fn(self) if applies else self

File ~/dev/examples/opensky/envs/default/lib/python3.11/site-packages/holoviews/core/operation.py:214, in Operation.__call__(self, element, **kwargs)
    210         return element.clone([(k, self._apply(el, key=k))
    211                               for k, el in element.items()])
    212     elif ((self._per_element and isinstance(element, Element)) or
    213           (not self._per_element and isinstance(element, ViewableElement))):
--> 214         return self._apply(element)
    215 elif 'streams' not in kwargs:
    216     kwargs['streams'] = self.p.streams

File ~/dev/examples/opensky/envs/default/lib/python3.11/site-packages/holoviews/core/operation.py:141, in Operation._apply(self, element, key)
    139     if not in_method:
    140         element._in_method = True
--> 141 ret = self._process(element, key)
    142 if hasattr(element, '_in_method') and not in_method:
    143     element._in_method = in_method

File ~/dev/examples/opensky/envs/default/lib/python3.11/site-packages/holoviews/operation/datashader.py:409, in aggregate._process(self, element, key)
    407 else:
    408     params['vdims'] = list(agg.coords[agg_fn.column].data)
--> 409     return ImageStack(agg, **params)

File ~/dev/examples/opensky/envs/default/lib/python3.11/site-packages/holoviews/element/raster.py:572, in ImageStack.__init__(self, data, kdims, vdims, **params)
    570     elif isinstance(data, dict):
    571         vdims = [Dimension(key) for key in data.keys() if key not in _kdims]
--> 572 super().__init__(data, kdims=kdims, vdims=vdims, **params)

File ~/dev/examples/opensky/envs/default/lib/python3.11/site-packages/holoviews/element/raster.py:279, in Image.__init__(self, data, kdims, vdims, bounds, extents, xdensity, ydensity, rtol, **params)
    276 else:
    277     params['rtol'] = config.image_rtol
--> 279 Dataset.__init__(self, data, kdims=kdims, vdims=vdims, extents=extents, **params)
    280 if not self.interface.gridded:
    281     raise DataError("{} type expects gridded data, {} is columnar. "
    282                     "To display columnar data as gridded use the HeatMap "
    283                     "element or aggregate the data (e.g. using rasterize "
    284                     "or np.histogram2d).".format(type(self).__name__, self.interface.__name__))

File ~/dev/examples/opensky/envs/default/lib/python3.11/site-packages/holoviews/core/data/__init__.py:325, in Dataset.__init__(self, data, kdims, vdims, **kwargs)
    322     if input_transforms is None:
    323         input_transforms = data._transforms
--> 325 kwargs.update(process_dimensions(kdims, vdims))
    326 kdims, vdims = kwargs.get('kdims'), kwargs.get('vdims')
    328 validate_vdims = kwargs.pop('_validate_vdims', True)

File ~/dev/examples/opensky/envs/default/lib/python3.11/site-packages/holoviews/core/dimension.py:116, in process_dimensions(kdims, vdims)
    111     elif not isinstance(dims, list):
    112         raise ValueError("{} argument expects a Dimension or list of dimensions, "
    113                          "specified as tuples, strings, dictionaries or Dimension "
    114                          "instances, not a {} type. Ensure you passed the data as the "
    115                          "first argument.".format(group, type(dims).__name__))
--> 116     dimensions[group] = [asdim(d) for d in dims]
    117 return dimensions

File ~/dev/examples/opensky/envs/default/lib/python3.11/site-packages/holoviews/core/dimension.py:116, in <listcomp>(.0)
    111     elif not isinstance(dims, list):
    112         raise ValueError("{} argument expects a Dimension or list of dimensions, "
    113                          "specified as tuples, strings, dictionaries or Dimension "
    114                          "instances, not a {} type. Ensure you passed the data as the "
    115                          "first argument.".format(group, type(dims).__name__))
--> 116     dimensions[group] = [asdim(d) for d in dims]
    117 return dimensions

File ~/dev/examples/opensky/envs/default/lib/python3.11/site-packages/holoviews/core/dimension.py:60, in asdim(dimension)
     50 def asdim(dimension):
     51     """Convert the input to a Dimension.
     52 
     53     Args:
   (...)
     58         copy is performed if the input is already a Dimension.
     59     """
---> 60     return dimension if isinstance(dimension, Dimension) else Dimension(dimension)

File ~/dev/examples/opensky/envs/default/lib/python3.11/site-packages/holoviews/core/dimension.py:276, in Dimension.__init__(self, spec, **params)
    273 all_params.update(params)
    275 if not all_params['name']:
--> 276     raise ValueError('Dimension name cannot be empty')
    277 if not all_params['label']:
    278     raise ValueError('Dimension label cannot be empty')

ValueError: Dimension name cannot be empty

:DynamicMap   []

Running the same code (I think!) with the old environment, it all works fine:

image

The dataset has an origin column we use to colorize the data. However, the whole column isn't filled with origins, there are some null values represented with an empty string. In the new environment, I can generate the plot by removing these empty origins, and suddenly these weird lines appear.

aggregator = ds.count_cat('origin')
datashaded = hd.datashade(hv.Path(flightpaths[flightpaths.origin != ''], ['longitude', 'latitude']), 
                          aggregator=aggregator, 
                          color_key=categorical_color_key(aggregator, 'hsv_r'))
datashaded.opts(width=850, height=600)

image

Removing all the empty origins isn't right as it also removes the lines with NaN values that indicate the separation between paths (that's the format of this dataset, paths are NaN separated in a table).

image

Removing the empty origins between the NaNs only, I stil get the same ValueError: Dimension name cannot be empty error:

aggregator = ds.count_cat('origin')
datashaded = hd.datashade(hv.Path(flightpaths[~((flightpaths.origin == '') & (~flightpaths.longitude.isna()))], ['longitude', 'latitude']), 
                          aggregator=aggregator, 
                          color_key=categorical_color_key(aggregator, 'hsv_r'))
datashaded.opts(width=850, height=600)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Azaya89 I think that's a pretty good hint at one issue we need to resolve for this example (and maybe others). There are some differences in behavior between the old and new HoloViews, and also between hvPlot and HoloViews. It'd be great if you could also look into reproducing these issues to confirm what I have found, and open issues where relevant.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you say old environments, do you mean the version of the dependencies before we updated them?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes exactly.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opened an issue related to that, it's likely there are more issues though holoviz/holoviews#6326.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Copy link
Contributor

@maximlt maximlt Nov 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Azaya89 the hvPlot code uses color while it should be color_key or cmap (not sure 🙃 ).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Azaya89, The current code is using by='origin', which under the hood creates a NdOverlay. Instead, I tried to use aggregator=ds.by('origin'), and it worked! In the sense that we no longer see these weird paths crossing the ocean.

flightpaths.hvplot.paths('longitude', 'latitude', tiles='EsriStreet', datashade=True, legend=False,
                         aggregator=aggregator, color_key=categorical_color_key(aggregator, 'hsv_r'))
image

It does mean there seems to be some weird behavior/bug with Paths and NdOverlay and datashade, it'd be nice if you could see if you can reproduce it with a simpler example (that doesn't require a huge dataset like this one).

"datashaded = hd.datashade(hv.Path(flightpaths, ['longitude', 'latitude']), \n",
" x_range=x_range, y_range=y_range, aggregator=ds.count_cat('ascending'))\n",
"hv.Tiles(tile_url) * datashaded"
"flightpaths.hvplot.paths('longitude', 'latitude', tiles='EsriStreet',\n",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's no longer blue and red :) But it should be.

Now:
image

Before:
image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Azaya89 same change, from by to aggregator. Also with explicitly mapping the value to its color and not showing the legend.

flightpaths.hvplot.paths('longitude', 'latitude', tiles='EsriStreet',
                         datashade=True, aggregator=ds.by('ascending'),
                         color_key={True: 'blue', False: 'red'}, legend=False)
image

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

opensky/opensky.ipynb Outdated Show resolved Hide resolved
" dynamic=False, **ranges).relabel(city)\n",
" for city, ranges in sorted(city_ranges.items())]).cols(3)"
"aggregator = ds.by('origin')\n",
"hv.Layout([flightpaths.hvplot.paths('longitude', 'latitude', dynamic=False, datashade=True,\n",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The formatting of this code cell is quite off, it needs to be reformatted to be easier to read.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. Done.

" dynamic=False, **ranges).relabel(city)\n",
" for city, ranges in sorted(city_ranges.items())]).cols(3)"
"aggregator = ds.by('origin')\n",
"hv.Layout([flightpaths.hvplot.paths('longitude', 'latitude', dynamic=False, datashade=True,\n",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The output looks pretty different.

Now:
image

Before:
image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Azaya89 again moving from by='origin' to aggregator=ds.by('origin') fixes this issue.

It'd be worth understanding why the colors are different. IIRC we saw that when an NdOverlay is created it's just looping over the default HoloViews colors?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

" aggregator=aggregator, \n",
" color_key=categorical_color_key(aggregator, 'hsv_r'),\n",
" dynamic=False, **city_ranges[\"Zurich\"]).opts(fig_size=400, bgcolor=None)"
"flightpaths.hvplot.paths('longitude', 'latitude', bgcolor=None, datashade=True,\n",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Different output.

Now:
image

Before:
image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Azaya89 same fix works:

flightpaths.hvplot.paths('longitude', 'latitude', bgcolor=None, datashade=True,
                         aggregator=ds.by('origin'), color_key=categorical_color_key(aggregator, 'hsv_r'),
                         **city_ranges["Zurich"])

@droumis
Copy link
Contributor

droumis commented Jun 26, 2024

@Azaya89 can you please click 'resolve conversation' for any comment that is resolved?

@Azaya89
Copy link
Collaborator Author

Azaya89 commented Jun 27, 2024

@Azaya89 can you please click 'resolve conversation' for any comment that is resolved?

Sure, I'll do that after I push the update, to avoid any confusion in case I click 'resolve' and you can't see it resolved in the PR.

Copy link
Contributor

Your changes were successfully integrated in the dev site, make sure to review
the pages of the projects you touched before merging this PR: https://holoviz-dev.github.io/examples/.
You can also download an archive of the site from the workflow summary page which comes in handy
when your dev site built was overriden by another PR (we have a single dev site!).

Copy link
Contributor

github-actions bot commented Nov 7, 2024

Your changes were successfully integrated in the dev site, make sure to review the pages of the projects you touched before merging this PR.

@Azaya89
Copy link
Collaborator Author

Azaya89 commented Nov 7, 2024

I think the only Issue now is the color disparity? @maximlt

Copy link
Contributor

@maximlt maximlt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like we can make good progress on this one. The difference between by=... and aggregator=... is unfortunate but let's catch it somewhere in an hvPlot issue if it's not yet done.

- notebook<7
- bokeh>=3.4.0
- colorcet>=3.1.0
- dask>=2023.11
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still to be done as far as I can see.

@@ -15,8 +15,6 @@
"\n",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still to be done as far as I can see.

@@ -15,8 +15,6 @@
"\n",
"Flight path information for commercial flights is available for some regions of the USA and Europe from the crowd-sourced [OpenSky Network](https://opensky-network.org/). OpenSky collects data from a large number of users monitoring public air-traffic control information. Here we will use a subset of the data that was polled from their REST API at an interval of 1 minute over 4 days (September 5-13, 2016), using the collect_data.py and prepare_data.py. In general the terms of use for OpenSky data do not allow redistribution, but we have obtained specific permission for distributing the subset of the data used in this project, which is a 200MB Parquet file (1.1GB as the original database). If you want more or different data, you can run the scripts yourself, or else you can contact OpenSky asking for a copy of the dataset.\n",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still to be done as far as I can see.

"datashaded = hd.datashade(hv.Path(flightpaths, ['longitude', 'latitude']), \n",
" x_range=x_range, y_range=y_range, aggregator=ds.count())\n",
"hv.Tiles(tile_url) * datashaded"
"flightpaths.hvplot.paths('longitude', 'latitude', tiles='EsriStreet',\n",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was a little mystery here with the cmap being different between the old and new plot.

old:
image

new:
image

HoloViews doesn't set a default value for cmap (https://github.com/holoviz/holoviews/blob/4a9e60990bd287e0f39b24d569f65b1ae459d4a0/holoviews/operation/datashader.py#L1164) so when not specified, which is the case in the old code, the default is obtained from datashader itself and is ["lightblue", "darkblue"] (https://github.com/holoviz/datashader/blob/05d8879fb142d879d304845dd672cfe68aa2faff/datashader/transfer_functions/__init__.py#L594). Apparently, datashader understands these names colors (lightblue and darkblue) and is able to create a colormap from this palette of two colors.

The new cmap value is 'kbc_r' and I think is set by hvPlot internally (https://github.com/holoviz/hvplot/blob/62c691f4f8c5781ac58f82173fcaa89de7a8ed4a/hvplot/converter.py#L882).

@Azaya89 ahh all the ways cmap and other color things can be set 🙃 Worth looking into the process_cmap in HoloViews (https://github.com/holoviz/holoviews/blob/4a9e60990bd287e0f39b24d569f65b1ae459d4a0/holoviews/plotting/util.py#L924).

" x_range=x_range, y_range=y_range, aggregator=ds.count())\n",
"hv.Tiles(tile_url) * datashaded"
"flightpaths.hvplot.paths('longitude', 'latitude', tiles='EsriStreet',\n",
" aggregator=ds.count(), datashade=True)"
]
},
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still to be done as far as I can see.

"outputs": [],
"source": [
"aggregator = ds.by('origin')\n",
"flightpaths.hvplot.paths('longitude', 'latitude', tiles='EsriStreet', datashade=True,\n",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Azaya89, The current code is using by='origin', which under the hood creates a NdOverlay. Instead, I tried to use aggregator=ds.by('origin'), and it worked! In the sense that we no longer see these weird paths crossing the ocean.

flightpaths.hvplot.paths('longitude', 'latitude', tiles='EsriStreet', datashade=True, legend=False,
                         aggregator=aggregator, color_key=categorical_color_key(aggregator, 'hsv_r'))
image

It does mean there seems to be some weird behavior/bug with Paths and NdOverlay and datashade, it'd be nice if you could see if you can reproduce it with a simpler example (that doesn't require a huge dataset like this one).

"datashaded = hd.datashade(hv.Path(flightpaths, ['longitude', 'latitude']), \n",
" x_range=x_range, y_range=y_range, aggregator=ds.count_cat('ascending'))\n",
"hv.Tiles(tile_url) * datashaded"
"flightpaths.hvplot.paths('longitude', 'latitude', tiles='EsriStreet',\n",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Azaya89 same change, from by to aggregator. Also with explicitly mapping the value to its color and not showing the legend.

flightpaths.hvplot.paths('longitude', 'latitude', tiles='EsriStreet',
                         datashade=True, aggregator=ds.by('ascending'),
                         color_key={True: 'blue', False: 'red'}, legend=False)
image

" dynamic=False, **ranges).relabel(city)\n",
" for city, ranges in sorted(city_ranges.items())]).cols(3)"
"aggregator = ds.by('origin')\n",
"hv.Layout([flightpaths.hvplot.paths('longitude', 'latitude', dynamic=False, datashade=True,\n",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Azaya89 again moving from by='origin' to aggregator=ds.by('origin') fixes this issue.

It'd be worth understanding why the colors are different. IIRC we saw that when an NdOverlay is created it's just looping over the default HoloViews colors?

" aggregator=aggregator, \n",
" color_key=categorical_color_key(aggregator, 'hsv_r'),\n",
" dynamic=False, **city_ranges[\"Zurich\"]).opts(fig_size=400, bgcolor=None)"
"flightpaths.hvplot.paths('longitude', 'latitude', bgcolor=None, datashade=True,\n",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Azaya89 same fix works:

flightpaths.hvplot.paths('longitude', 'latitude', bgcolor=None, datashade=True,
                         aggregator=ds.by('origin'), color_key=categorical_color_key(aggregator, 'hsv_r'),
                         **city_ranges["Zurich"])

Copy link
Contributor

Your changes were successfully integrated in the dev site, make sure to review the pages of the projects you touched before merging this PR.

@Azaya89
Copy link
Collaborator Author

Azaya89 commented Nov 18, 2024

Implemented all the fixes and I think it looks good now @maximlt

@maximlt
Copy link
Contributor

maximlt commented Nov 20, 2024

Implemented all the fixes and I think it looks good now @maximlt

@Azaya89 thanks for the changes. I've just pushed some minor changes:

  • Some code lines were too wide and would be difficult to read on mobile
  • Fixed the aspect of some plots
  • Used title=... instead of .relabel()

Can you please review these changes?

Copy link
Collaborator Author

@Azaya89 Azaya89 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Contributor

Your changes were successfully integrated in the dev site, make sure to review the pages of the projects you touched before merging this PR.

@maximlt maximlt merged commit c167fc0 into main Nov 21, 2024
9 checks passed
@Azaya89 Azaya89 deleted the modernize_opensky branch November 25, 2024 09:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NF SDG NumFocus Software Development Grant 2024
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OpenSky example broken
3 participants