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

Get the "Interactive Plots" to work and show up #29

Merged
merged 23 commits into from
Dec 5, 2023
Merged

Conversation

erogluorhan
Copy link
Member

@erogluorhan erogluorhan commented Nov 29, 2023

This PR mainly uses the mpas-datashader.ipynb, making some changes in it to cover interactive plotting in this cookbook and emphasize there were some updates in unstructured grids visualization via UXarray since the initial version of this study was conducted.

This PR does:

  • Rename "mpas-datashader.ipynb" to "interactive-holoviz-mpas"
  • Revise the text to handle updated information on unstructured grids/interactive visualization
  • Add an "important" admonition into the beginning of the notebook about UXarray cookbook (Link to it is currently 404 but will hopefully work after that cookbook is published)
  • Add tips to encourage interactivity with the plots
  • Swap the order of "Interactive plots" and "Animation" in the cookbook, i.e. the former is the final chapter now in the cookbook

Copy link

github-actions bot commented Nov 29, 2023

👋 Thanks for opening this PR! The Cookbook will be automatically built with GitHub Actions. To see the status of your deployment, click below.
🔍 Git commit SHA: 74d8c55
✅ Deployment Preview URL: https://ProjectPythia.github.io/advanced-viz-cookbook/_preview/29

@jukent
Copy link
Collaborator

jukent commented Nov 30, 2023

Thanks for fixing the toc! Looks like we want to add `'geoviews' to the environment.

@anissa111
Copy link
Member

Should this section go after animation instead of before? If not, the spaghetti plots notebook should point to this notebook in the "whats next" section I think

@jukent
Copy link
Collaborator

jukent commented Nov 30, 2023

Yes the "what's next" sections will need to be corrected. I agree we could put this after animation.

@erogluorhan erogluorhan requested a review from jukent December 2, 2023 16:15
@erogluorhan erogluorhan marked this pull request as ready for review December 2, 2023 16:15
@philipc2
Copy link

philipc2 commented Dec 2, 2023

While the flow and content of this notebook is good, I think it misses the point of being used to showcase the interactive functionality of HoloViews.

A think there should be a much greater emphasis put into explaining the actual interactivity of these plots. A few suggestions

  • Reduce the amount of non-interactive visualization. At the minimum, I'd exclude the "Synthesizing triangles from points using Delaunay triangulation" section
  • There is still too many comments in each code cell
  • Can you provide an example to where setting dynamic to True or False makes a difference
  • Describe what the .opts accessor is doing, what is the point of doing tools=['hover']

What do you think?

@erogluorhan
Copy link
Member Author

Should this section go after animation instead of before? If not, the spaghetti plots notebook should point to this notebook in the "whats next" section I think

Yes the "what's next" sections will need to be corrected. I agree we could put this after animation.

Yes, I have addressed these both

@erogluorhan
Copy link
Member Author

While the flow and content of this notebook is good, I think it misses the point of being used to showcase the interactive functionality of HoloViews.

A think there should be a much greater emphasis put into explaining the actual interactivity of these plots. A few suggestions

  • Reduce the amount of non-interactive visualization. At the minimum, I'd exclude the "Synthesizing triangles from points using Delaunay triangulation" section
  • There is still too many comments in each code cell
  • Can you provide an example to where setting dynamic to True or False makes a difference
  • Describe what the .opts accessor is doing, what is the point of doing tools=['hover']

What do you think?

These reviews are invaluable! I agree with all of them and will be working on getting them addressed.

@philipc2
Copy link

philipc2 commented Dec 2, 2023

While the flow and content of this notebook is good, I think it misses the point of being used to showcase the interactive functionality of HoloViews.

A think there should be a much greater emphasis put into explaining the actual interactivity of these plots. A few suggestions

  • Reduce the amount of non-interactive visualization. At the minimum, I'd exclude the "Synthesizing triangles from points using Delaunay triangulation" section
  • There is still too many comments in each code cell
  • Can you provide an example to where setting dynamic to True or False makes a difference
  • Describe what the .opts accessor is doing, what is the point of doing tools=['hover']

What do you think?

These reviews are invaluable! I agree with all of them and will be working on getting them addressed.

Fantastic! One suggestion to keep in mind

For the sections of the notebook, what do you think of having it broken down into the following Sub Titles


HoloViz

Overview

Content here would be mostly unchanged

Trimesh Visualization

Content would almost be mostly unchanged, however slightly condensed, as per the points I gave above.

Interactivity

New content to this notebook, would manipulate the plots from the Trimesh Visualuzation section to showcase the interactive features of HoloViews.

@erogluorhan
Copy link
Member Author

erogluorhan commented Dec 4, 2023

Hey @philipc2, this is my responses to both of your comments above:

While the flow and content of this notebook is good, I think it misses the point of being used to showcase the interactive functionality of HoloViews.

It now emphasizes interactivity more (even though I kept the primal and dual mesh parts for not only getting the data ready for visualization but also keeping them as reference for the future. However, I now name those parts as "preprocessing" that suggests it's not direclt visualization-related)

  • Dynamic rasterization (i.e. dynamic=True) is only mentioned in the admonition, however this is one of the most important parameters for interactive visualization at high-resolution (my discussion from the UXarray cookbook)

We now have a separate part dedicated to its discussion

  • No mention to panels, additional hover tools, HoloMaps, or other HoloViz interactive functionality (there is a lot more to interactive plotting than just panning, unless that is the aim of this notebook), a few examples

These are great recommendations, though I thought discussing mandelbrot, panel, HoloMap could expand the scope too much, so I included them as "further reading" recommendations in the text.

  • Personally, it still feels like too much of the content of this notebook is dedicated to "Unstructured Grid Visualization", with the only "interactive" elements of this notebook being the outputted plots. There is also no "interactive" difference between the three rendered plots

Thanks very much for this; it was much helpful for me to rethink about the flow and content. Now, interactivity has a whole section of it under which several subjects are discussed. Though, I still would like to keep primal and dual mesh parts as "preprocessing" because of the reasons mentioned in the beginning of this comment

A think there should be a much greater emphasis put into explaining the actual interactivity of these plots. A few suggestions

  • Reduce the amount of non-interactive visualization. At the minimum, I'd exclude the "Synthesizing triangles from points using Delaunay triangulation" section

Done; thanks a lot!

  • There is still too many comments in each code cell

Instead of removing the code cell comments, I had added the following admonition to explain for what purpose comments in the code cells would work:

image

What do you think?

  • Can you provide an example to where setting dynamic to True or False makes a difference

Added the "Dynamic plot" section to discuss this.

  • Describe what the .opts accessor is doing, what is the point of doing tools=['hover']

Added the "Holoviews’ options system: opts" section to discuss this.

For the sections of the notebook, what do you think of having it broken down into the following Sub Titles

HoloViz

Overview

Content here would be mostly unchanged

Trimesh Visualization

Content would almost be mostly unchanged, however slightly condensed, as per the points I gave above.

Interactivity

New content to this notebook, would manipulate the plots from the Trimesh Visualuzation section to showcase the interactive features of HoloViews.

Sorry that I had missed this comment of yours until this morning and I had revised the notebook significantly to get its latest version up as of last night. However, I feel like you and I thought in a very similar way. Here is the new titles/subtitles with the latest version:

#Holoviz

Overview

MPAS preprocessiing

Utility functions

Data loading

Triangular mesh generation using MPAS’s cell connectivity array from the primal mesh

Interactive Holoviz Plots

Holoviews’ options system: opts

Pan & zoom

Hover

Dynamic plot

Further Interactivity Considerations

What do you think?

@jukent
Copy link
Collaborator

jukent commented Dec 4, 2023

Couple minor notes:

  • add yourself (and John? Philip?) as an author in the README and CITATION.cff
  • in the first admonition "we highly recommend to see also" should be changed to something like "we highly recommend checking out" or "we highly recommend looking at" for grammar

will add more notes after lunch!

Copy link

@philipc2 philipc2 left a comment

Choose a reason for hiding this comment

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

The changes look wonderfull, the flow is significantly better now.

I've compiled some comments here, let me know if you can access it

https://app.reviewnb.com/philipc2/advanced-viz-cookbook/blob/interactive_plots/notebooks%2Finteractive-holoviz-mpas.ipynb

A few general comments

  • Check for grammar throughout
  • Please run the pre-commit on the notebook to fix the formatting of some of the cells, and generally try to keep code comments to a minimal (use docstrings for function descriptions, move descriptive code into markdown cells, etc.)

@erogluorhan erogluorhan requested a review from philipc2 December 4, 2023 20:49
@erogluorhan
Copy link
Member Author

Couple minor notes:

  • add yourself (and John? Philip?) as an author in the README and CITATION.cff
  • in the first admonition "we highly recommend to see also" should be changed to something like "we highly recommend checking out" or "we highly recommend looking at" for grammar

will add more notes after lunch!

Great catches; thanks! Addressed these all and once the build succeeds, you can give it another look

@erogluorhan
Copy link
Member Author

I've compiled some comments here, let me know if you can access it
https://app.reviewnb.com/philipc2/advanced-viz-cookbook/blob/interactive_plots/notebooks%2Finteractive-holoviz-mpas.ipynb

Yeah, thanks! I have addressed them all, and they were great to improve things significantly.

  • Check for grammar throughout

I believe it is completed now.

  • Please run the pre-commit on the notebook to fix the formatting of some of the cells, and generally try to keep code comments to a minimal (use docstrings for function descriptions, move descriptive code into markdown cells, etc.)

Actually pre-commit gets angry with several other notebooks too, so @jukent may hav a better understanding on what is going on there.

Copy link

@philipc2 philipc2 left a comment

Choose a reason for hiding this comment

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

Looking good!

Only thing that appears to be missing is a note/warning regarding the "dynamic" option not actually being dynamic in the rendered notebook.

@erogluorhan
Copy link
Member Author

Looking good!

Only thing that appears to be missing is a note/warning regarding the "dynamic" option not actually being dynamic in the rendered notebook.

Ok, I can do that, but instead of that, do you think it'd be worth making all the prior plots with dynamic=False and just start doing them True dynamic here so that the plot would comply with the title better?

@philipc2
Copy link

philipc2 commented Dec 4, 2023

Looking good!
Only thing that appears to be missing is a note/warning regarding the "dynamic" option not actually being dynamic in the rendered notebook.

Ok, I can do that, but instead of that, do you think it'd be worth making all the prior plots with dynamic=False and just start doing them True dynamic here so that the plot would comply with the title better?

That's a good idea also. My concern however is if someone zooms into the "dynamic" plot via the cookbook, it won't actually re-render anything.

@philipc2
Copy link

philipc2 commented Dec 4, 2023

Need to still add the "Interactive" section to the "Structure" section of the README.

@philipc2
Copy link

philipc2 commented Dec 4, 2023

For the "Hover" section, can you modify the clim to center the colorbar about 0?

clim=(-4e-4, 4e-4) should do the trick

@jukent
Copy link
Collaborator

jukent commented Dec 5, 2023

This is looking really strong! Everything I noticed is already captured by @philipc2 's comments

@erogluorhan
Copy link
Member Author

"dynamic" option not actually being dynamic in the rendered notebook

Oops, with this comment, I just got what you meant with that warning about dynamic plots in rendered notebooks earlier today. Okay, I will put a warning about this

@erogluorhan
Copy link
Member Author

I believe I have addressed all thelatest things suggested. Namely:

Only thing that appears to be missing is a note/warning regarding the "dynamic" option not actually being dynamic in the rendered notebook.

Need to still add the "Interactive" section to the "Structure" section of the README.

For the "Hover" section, can you modify the clim to center the colorbar about 0? clim=(-4e-4, 4e-4) should do the trick

@erogluorhan erogluorhan requested a review from philipc2 December 5, 2023 03:08
@philipc2
Copy link

philipc2 commented Dec 5, 2023

I believe I have addressed all thelatest things suggested. Namely:

Only thing that appears to be missing is a note/warning regarding the "dynamic" option not actually being dynamic in the rendered notebook.

Need to still add the "Interactive" section to the "Structure" section of the README.

For the "Hover" section, can you modify the clim to center the colorbar about 0? clim=(-4e-4, 4e-4) should do the trick

These additions are perfect!

Copy link

@philipc2 philipc2 left a comment

Choose a reason for hiding this comment

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

Almost there! A few more minor suggestions

  • Refer to Holoviz as HoloViz for consistency throughout
  • The title "HoloViz Tools" could be rephrased, I'm not a big fan of using the word "Tools", maybe "HoloViz Packages" (Technically, the only HoloViz package used in this notebook is HoloViews, so we should maybe reconsider the use of HoloViz throughout). Any thoughts?

@erogluorhan
Copy link
Member Author

Almost there! A few more minor suggestions

  • Refer to Holoviz as HoloViz for consistency throughout
  • The title "HoloViz Tools" could be rephrased, I'm not a big fan of using the word "Tools", maybe "HoloViz Packages" (Technically, the only HoloViz package used in this notebook is HoloViews, so we should maybe reconsider the use of HoloViz throughout). Any thoughts?
  • Refer to Holoviz as HoloViz for consistency throughout

Done

  • The title "HoloViz Tools" could be rephrased, I'm not a big fan of using the word "Tools", maybe "HoloViz Packages" (Technically, the only HoloViz package used in this notebook is HoloViews, so we should maybe reconsider the use of HoloViz throughout). Any thoughts?

HoloViz Plots, HoloViz Functions, or HoloViz Functionality? I lean towards HoloViz Plots. I think HoloViz Packages would also work as we are using geoviews, datashader through holoviews.opereations despite not directly for interactivity.

Originally I wanted to use "HoloViz Visualizations" but it sounded duplicative right under the chapter name "Interactive Visualization ...".

@erogluorhan erogluorhan requested a review from philipc2 December 5, 2023 15:11
@erogluorhan
Copy link
Member Author

Hey @philipc2 I have pushed the renaming of chapter and the notebook as we discussed, so this should good for another review

Copy link

@philipc2 philipc2 left a comment

Choose a reason for hiding this comment

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

Looks great! Gave it a final read through and all looks good.

@erogluorhan
Copy link
Member Author

Looks great! Gave it a final read through and all looks good.

Thanks very much for the thorough review!

@erogluorhan erogluorhan merged commit 07a8a07 into main Dec 5, 2023
1 of 2 checks passed
github-actions bot pushed a commit that referenced this pull request Dec 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants