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: heat_and_trees #395

Merged
merged 14 commits into from
Jul 16, 2024
Merged

Modernize notebook: heat_and_trees #395

merged 14 commits into from
Jul 16, 2024

Conversation

Azaya89
Copy link
Collaborator

@Azaya89 Azaya89 commented May 29, 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 29, 2024
@Azaya89 Azaya89 requested a review from maximlt May 29, 2024 21:15
@Azaya89 Azaya89 self-assigned this May 29, 2024
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 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!).

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
Copy link
Collaborator Author

Azaya89 commented Jun 13, 2024

Comments on the warnings displayed:

FutureWarning: The return type of `Dataset.dims` will be changed to return a set of dimension names in future, in order to be more consistent with `DataArray.dims`. To access a mapping from dimension names to lengths, please use `Dataset.sizes`.

This particular warning message indicates an upcoming change in the behavior of the Dataset.dims property in a future version of the xarray library. However, the version of xarray currently in use in this PR already returns the set of dimension names when Dataset.dims is checked, which renders this warning redundant.

OMP: Info #276: omp_set_nested routine deprecated, please use omp_set_max_active_levels instead.

There is an open PR here that should fix the warning.

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!).

@maximlt
Copy link
Contributor

maximlt commented Jun 19, 2024

FutureWarning: The return type of `Dataset.dims` will be changed to return a set of dimension names in future, in order to be more consistent with `DataArray.dims`. To access a mapping from dimension names to lengths, please use `Dataset.sizes`.

This particular warning message indicates an upcoming change in the behavior of the Dataset.dims property in a future version of the xarray library. However, the version of xarray currently in use in this PR already returns the set of dimension names when Dataset.dims is checked, which renders this warning redundant.

The full warning I have is:

/Users/mliquet/dev/examples/heat_and_trees/envs/default/lib/python3.11/site-packages/intake_xarray/raster.py:108: FutureWarning: The return type of Dataset.dims will be changed to return a set of dimension names in future, in order to be more consistent with DataArray.dims. To access a mapping from dimension names to lengths, please use Dataset.sizes.
'dims': dict(ds2.dims),

You'll notice it is attributed to line 108 in the file intake_xarray/raster.py. I put a breakpoint in the code to debug it and print the variables ds2.dims (the one causing the warning) and ds2.sizes (the one recommended for update):

ipdb> ds2.dims
FrozenMappingWarningOnValuesAccess({'x': 7741, 'y': 7871, 'band': 4})
ipdb> ds2.sizes
Frozen({'x': 7741, 'y': 7871, 'band': 4})

At the moment, with version 2024.6.0 of xarray, ds2.dims return a mapping from dimension names to their length. The warning explains this behavior is going to change in the future, ds2.dims will return a set object that only includes the dimension names. Let's now have a look at intake-xarray's code to see what it does withds2.dims:

            metadata = {
                'dims': dict(ds2.dims),
                'data_vars': {k: list(ds2[k].coords)
                              for k in ds2.data_vars.keys()},
                'coords': tuple(ds2.coords.keys()),
                'array': 'raster'
            }

You can see it calls dict(ds2.dims) which basically just casts the special mapping object returned by ds2.dims to a standard Python dictionary. So in the future, when ds2.dims returns just a set object, this code will break! As such, intake-xarray needs to be updated! Can you please open an issue on the Github repository of intake-xarray to report this problem, and open a PR to fix it? I think updating to 'dims': dict(ds2.sizes) should be enough, assuming the sizes property has been there long enough.

However, the version of xarray currently in use in this PR already returns the set of dimension names when Dataset.dims is checked, which renders this warning redundant.

Yes but part of the exercise was to open issues in other projects to let them know that their library emits a warning when used.

heat_and_trees/heat_and_trees.ipynb Outdated Show resolved Hide resolved
heat_and_trees/anaconda-project.yml Show resolved Hide resolved
heat_and_trees/anaconda-project.yml Show resolved Hide resolved
heat_and_trees/anaconda-project.yml Show resolved Hide resolved
heat_and_trees/anaconda-project.yml Outdated Show resolved Hide resolved
heat_and_trees/heat_and_trees.ipynb Outdated Show resolved Hide resolved
heat_and_trees/heat_and_trees.ipynb Outdated Show resolved Hide resolved
heat_and_trees/heat_and_trees.ipynb Outdated Show resolved Hide resolved
heat_and_trees/heat_and_trees.ipynb Outdated Show resolved Hide resolved
@Azaya89
Copy link
Collaborator Author

Azaya89 commented Jun 19, 2024

At the moment, with version 2024.6.0 of xarray, ds2.dims return a mapping from dimension names to their length. The warning explains this behavior is going to change in the future, ds2.dims will return a set object that only includes the dimension names. Let's now have a look at intake-xarray's code to see what it does withds2.dims:

I'm still a bit confused by this 'cos this is what ds.dims returns for me:
Screenshot 2024-06-19 at 11 19 16 AM

which tells me that it's already exhibiting the future behaviour. Is there something I'm missing?

@Azaya89
Copy link
Collaborator Author

Azaya89 commented Jun 19, 2024

I'm also getting this same error when I run doit validate:..., should I just ignore it or is there a way to resolve it currently?

@Azaya89 Azaya89 requested a review from maximlt June 19, 2024 11:38
@Azaya89
Copy link
Collaborator Author

Azaya89 commented Jun 27, 2024

I created an Issue here intake/intake-xarray#145 concerning the FutureWarning, although I can see that the last update in that repo was 9 months ago...

@maximlt
Copy link
Contributor

maximlt commented Jun 27, 2024

I created an Issue here intake/intake-xarray#145 concerning the FutureWarning, although I can see that the last update in that repo was 9 months ago...

Cool thanks! Can you now fork the repo and open a PR to fix this issue as suggested in #395 (comment)?

@maximlt
Copy link
Contributor

maximlt commented Jun 27, 2024

I can see that the last update in that repo was 9 months ago...

Some projects don't need much update :)

@Azaya89
Copy link
Collaborator Author

Azaya89 commented Jun 28, 2024

Cool thanks! Can you now fork the repo and open a PR to fix this issue as suggested in #395 (comment)?

Done here

@maximlt
Copy link
Contributor

maximlt commented Jun 30, 2024

Cool thanks! Can you now fork the repo and open a PR to fix this issue as suggested in #395 (comment)?

Done here

Thanks! I see it got already merged 👍

Copy link
Contributor

github-actions bot commented Jul 2, 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 Jul 2, 2024

@Azaya89 I made some updates to move the download of the two GeoJSON files to the intake catalog. I know we said we wanted to move away from intake but this is more work than I'm ready to put in at the moment, IMO we'll have to stick with intake for now.

@Azaya89
Copy link
Collaborator Author

Azaya89 commented Jul 2, 2024

Is this PR ready to be merged now @maximlt ?

Copy link
Contributor

github-actions bot commented Jul 2, 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 Jul 2, 2024

Is this PR ready to be merged now @maximlt ?

I updated the branch to get a dev site build and check how it looks. If it's okay, I'll merge it.

Copy link
Contributor

github-actions bot commented Jul 2, 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!).

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.

Probably need to set cnorm to get the same output as before.

Before:
image

Now:
image

@Azaya89
Copy link
Collaborator Author

Azaya89 commented Jul 3, 2024

Probably need to set cnorm to get the same output as before.

Done

@Azaya89 Azaya89 requested a review from maximlt July 3, 2024 09:22
Copy link
Contributor

github-actions bot commented Jul 3, 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!).

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!).

@maximlt maximlt merged commit 75d8ab1 into main Jul 16, 2024
9 checks passed
@maximlt maximlt deleted the modernize_heat_trees branch October 8, 2024 09:54
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.

2 participants