-
Notifications
You must be signed in to change notification settings - Fork 25
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
NYC_buildings: Modernize notebook #386
base: main
Are you sure you want to change the base?
Conversation
Your changes were successfully integrated in the dev site, make sure to review |
17121aa
to
9b6c32c
Compare
In this PR, pinning |
Can you please add more details on this? |
Ok it looks like a packaging issue. I don't understand why |
@Azaya89 can you maybe try to pin again |
This is not able to work because |
I would like to see it failing on the CI to see if it reports the same error that you get. |
nyc_buildings/anaconda-project.yml
Outdated
@@ -15,20 +15,26 @@ user_fields: [examples_config] | |||
|
|||
channels: | |||
- defaults |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Azaya89 ah I just noticed something. It's good practice not to mix the defaults
channel with conda-forge
. So when we use conda-forge
we should replace defaults
with nodefaults
, to avoid the defaults
channel to be added by default 🙃 Can you try that on your machine and re-lock?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, but there was no failure in the CI here. This is diabolical!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
Your changes were successfully integrated in the dev site, make sure to review |
1 similar comment
Your changes were successfully integrated in the dev site, make sure to review |
Your changes were successfully integrated in the dev site, make sure to review |
fe3b8fb
to
b2030b9
Compare
Your changes were successfully integrated in the dev site, make sure to review |
@maximlt I think this PR is ready for another review with the following notes:
|
Ok thanks for the report. Depending on the performance issues, it might be that we end up not updating the code in this example. |
:( |
Isaiah reports that it takes about 30 seconds to run a cell with the full visualization with geopandas and that now (reverting back to spatialpandas on his local machine) it's taking even longer. |
b2030b9
to
3983383
Compare
3983383
to
11c8dcf
Compare
The test is failing because of this fix that is not merged yet. |
nyc_buildings/nyc_buildings.ipynb
Outdated
}, | ||
"outputs": [], | ||
"source": [ | ||
"ddf.hvplot.polygons(rasterize=True, tiles='CartoLight', groupby='type', aggregator='any')" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before, in Jim's example, the categories in the widget were alphabetically sorted. It's no longer the case:
Jim's code used an intermediate HoloMap
, this object has a sort
keyword that is True by default:
In hvPlot, groupby=True
leads internally to applying the groubpy
operation method of hv.Dataset
. It's called with dynamic=True
(the default) which means a DynamicMap is returned and not a HoloMap, loosing that default sorting behavior.
We need to open an issue on hvPlot I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would the Issue address? A way to alphabetize a hvPlot groupby
operation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but more generally a way to declare that the values of the grouped dimension(s) should be sorted. It should be possible to come up with a small example to reproduce this.
In our case, we could ignore this, or preprocess the dataset in the notebook to sort it by category.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be clear, I've contributed to this example, but it's from @philippjfr , not me! :-)
nyc_buildings/nyc_buildings.ipynb
Outdated
"hover = inspect_polygons(shaded).opts(fill_color='red', tools=['hover'])\n", | ||
"\n", | ||
"tiles * shaded * legend * hover" | ||
"plot = ddf.hvplot.polygons(tiles='CartoLight', data_aspect=1, datashade=True,\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I can. I think it had something to do with setting data_aspect=1
Here's the same plot without setting the data aspect:
nyc_b_gif720.mov
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok yes that's better without it. In fact, I'm not sure you can modify the display when tiles are used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, I will remove the data_aspect
parameter then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. Mixing data_aspect=1 and tiles is at best redundant, and at worst sets them up for a death match with each other.
Have the duplicated legend entries visible in #386 (comment) been addressed? Previously that's been due to having both manual and automatic legends in the same plot. |
Yes, this PR does not have duplicate legend entries. |
Blocked by holoviz/holoviews#6470, I found a pretty bad performance regression with the modernized code when creating the groupby rasterized plot, while creating the HoloViews object itself, see the PR for more details. For @Azaya89, I created an environment from Jim's previous work you ported to this PR (Closed). On this branch, the lock file is not recent and so not ready for osx-arm64. A while back I installed https://orbstack.dev/ on my machine, it makes it very easy to spin up a Linux machine on Mac, and you can still navigate through your normal files, very handy. This allowed me to prepare the project from Jim's branch and run the notebook, with some modifications to time it ( Comparing old vs new with holoviz/holoviews#6470:
Only the plot 2 takes longer to display, the difference lies in the HoloViews object creation (0.6s in the new code):
So I think in terms of performance, at least for the first render, we're all good with the changes. |
I've opened holoviz/holoviews#6471 and holoviz/hvplot#1463 to follow-up on #386 (comment), i.e. allow sorting the values of the groupby dimensions. I don't consider it a blocker and think it needs some discussion at the hvplot and holoviews levels, so I'd say we can move on. |
Thank @Azaya89 for re-locking but:
No need for anyone to review this example until these two points above are resolved, and the CI is green :) |
Modernizing an example checklist
Preliminary checks
Change ‘anaconda-project.yml’ to use the latest workable version of packages
hvplot<0.9
tohvplot
,panel>=0.12,<1.0
topanel>=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.hvplot
tohvplot>=0.9.2
,hvplot>=0.8
tohvplot>=0.9.2
). Usually, the new/updated lower pin of a dependency will be the version resolved afteranaconda prepare
has been run. Execute!conda list
in a notebook, oranaconda 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.Plot API updates (discussed on a per-example basis)
datashade
withrasterize
(read this page). Essentially,rasterize
allows Bokeh to handle the colormapping instead of Datashader.Interactivity API updates (discussed on a per-example basis)
pn.interact
usage.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.pn.bind()
. Read this page for explanation.view()
method and call it directly, update the class by inheriting frompn.viewable.Viewer
and replaceview()
by__panel__()
. Here is an example.Panel App updates (discussed on a per-example basis)
pn.Column
, or more complicated to incorporate widgets, etc. Make the final app.servable()
.command: dashboard
declaration in theanaconda-project.yml
file), try adding it.template = pn.template.BootstrampTemplate
, but if building up an app across multiple cells, it is probably cleaner to declare the template at the top withpn.extension(template='bootstrap')
. See how to guide on setting a template.General code quality updates
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
Visual appearance - Example
Visual appearance - Gallery
Ml Annotators
toML Annotators
), if not, add/update theexamples_config.title
field inanaconda-project.yml
description
field inanaconda-project.yml
Workflow (after you have made the changes above)
doit validate:<projectname>
doit test:<projectname>
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.