Adding beginner friendly explanations on the Docs - #1607
Open
dylannelson wants to merge 7 commits into
Open
Conversation
dylannelson
added
documentation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
ASV BenchmarkingBenchmark Comparison ResultsBenchmarks that have improved:
Benchmarks that have stayed the same:
Benchmarks that have got worse:
|
Sevans711
requested changes
Jul 22, 2026
Sevans711
left a comment
Sevans711
left a comment
Collaborator
There was a problem hiding this comment.
Thank you for proposing and creating these additions to the docs pages! Overall, these all look like good changes which can help new users understand uxarray more easily.
I have a few miscellaneous questions / suggestions / requests:
- For the iso_grid.png, can the image be shrunk to be smaller without losing too much image quality? The docs are currently roughly 28 MB, so it's not a huge deal to add 1.5 MB, but if it wouldn't make the image too blurry it could be nice to reduce file size further.
- This phrasing was a bit confusing for me "Grid points store information about which faces, edges, and nodes they are connected to…" What are "grid points"? Additionally, it is not the grid points themselves which store such information, but rather the information is stored in the underlying uxarray.Grid object, if that makes sense?
- The unstructured_grid.png diagram bottom row was a bit confusing, in particular, why is there more empty space between hexagons there than above? In the structured_grid.png I can interpret the empty space between faces as being placed there for visual emphasis, to help talk about the different faces. I could interpret it that way for the unstructured_grid.png too if the empty space was consistent, but now that it is inconsistent I am not sure what to think.
- Optional suggestion: add a non-hexagon face in the unstructured_grid diagram to emphasize that "number of nodes per face" can also vary.
- Claims at the bottom of the new unstructured-grids.html about unstructured grids granting efficiency improvements are nice, and okay to move forward as-is, but may be more convincing with links to any relevant publications or resources providing direct evidence for unstructured grids' improvements over structured grids in practice. Maybe @erogluorhan and @rajeeja could provide suggestions about that?
- In data-structures.ipynb, maybe use the spelling "UxDataArray" instead of "Data Array"? Additionally, maybe edit the description of uxarray.UxDataArray to not call itself a data variable? It could say something like "A single array of data residing on the faces, nodes, or edges of a grid, along with the underlying Grid object."
- It looks like you tried to change the color for Yes and No cells in grid-formats.rst, but the tables still render with the same exact colors when I viewed them. Is this intentional?
- Can you explicitly clarify which changes solve which parts of issue #1582? For example, Point (2) of that issue refers to user-guide/representation, but I noticed none of the changes here touch that file. Is point (2) solved elsewhere, or still needs to be solved before that issue could be closed as completed?
Collaborator
Author
|
@Sevans711 Good notes! And thank you for the through read!
|
rljacob
reviewed
Jul 23, 2026
Collaborator
|
Thank you for your detailed reply @dylannelson, the ideas here sound good, and I will take a closer look again once the changes are ready! In the meantime, following up on a few of those points:
|
erogluorhan
reviewed
Jul 23, 2026
erogluorhan
left a comment
erogluorhan
left a comment
Member
There was a problem hiding this comment.
This is going great! I've added a few inline comments, and they refer to another document from our cookbooks. Once you review it, and if you want to modify anything in your comparison file here, once you're done with that, I can give another review on that file. Also:
- If possible, add some randomness and break the symmetry in unstructured_grid.png, at least removing one of the two pentagons?
|
|
||
| ================================ | ||
| Structured vs Unstructured Grids | ||
| ================================ |
Member
There was a problem hiding this comment.
For this section and others in the rest of this file, you may want to check out this section from our UXarray Cookbook: https://projectpythia.org/unstructured-grid-viz-cookbook/notebooks/foundations/unstructured-grids/#structured-vs-unstructured-grids since a lot of text there can be reused here. However, it touched code etc. which may not be needed here.
| :width: 300 | ||
| :align: center | ||
| :alt: A structured grid with a regular, matrix-like arrangement of cells | ||
|
|
Member
There was a problem hiding this comment.
While it is technically not false/impossible to define cells being centered on a particular (lon,lat) pair, grid cells in structured grids are defined with their boundary longitudes and latitudes as follows:
So, rather than identifying cell centers with lon, lat pairs, maybe draw a diagram similar to the above but with only nine cells, tag the constant lon and lat lines instead of cell centers, and rephrase the text in this section accordingly?
Collaborator
Author
There was a problem hiding this comment.
I think I was able to capture everything here in my last push, let me know if it captures the vision you had. Tried to keep the same styling as before, fairly minimal and as much glance value as possible
Added content based on feedback from team and many tweaks to the images
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1582
Overview
While reading through the docs, there was a few ideas I reviewed that could be useful to new users. I was able to implement a few and test the changes
1) More examples/distinctions between unstructured vs structured grids
uxarray\docs\user-guide\unstructured-grids.rst2) Updating Tutorials and Videos Page
docs/tutorials.rst3) More distinct defintions for UxDataAray vs UxDataset
PR Checklist
Documentation