Adding beginner friendly explanations on the Docs#1607
Open
dylannelson wants to merge 5 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:
|
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
| Use Cases | ||
| ========= | ||
|
|
||
| Many kinds of unstructured grids can exist because of these features. |
Member
There was a problem hiding this comment.
A main reason for unstructured grids is to avoid the singularity at the poles in a lat-lon grid.
Collaborator
Author
There was a problem hiding this comment.
Great point. I'll definitely include that. Thank you!
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?
| @@ -0,0 +1,124 @@ | |||
| .. currentmodule:: uxarray | |||
Member
There was a problem hiding this comment.
How about moving this file from "Getting Started" to the first place of "User Guide" because the former is designed as a quick reference for getting the user to get going with UXarray, e.g. install & run it?
|
|
||
| ================================ | ||
| 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.
| ================================ | ||
|
|
||
| UXarray's ability to work on such a large variety of datasets and file formats | ||
| comes from its support for unstructured grids. Unstructured grids differ from |
Member
There was a problem hiding this comment.
The first sentence feels a bit off-placed in this section. Maybe it can be replaced with something else that starts the actual subject of structured vs unstructured comparison?
For example, this is how the referenced cookbook above begins with: "Before diving into unstructured grids, it is helpful to understand the basic differences between structured and unstructured grids"
| .. image:: ../_static/examples/grids/ocean.png | ||
| :width: 300 | ||
| :align: center | ||
| :alt: An unstructured ocean grid with holes cut out over land regions |
Member
There was a problem hiding this comment.
This image is being rendered too small in the doc:
| :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?
| navigated predictably because it follows a repeatable pattern. | ||
|
|
||
| Example | ||
| ------- |
Member
There was a problem hiding this comment.
This is already a small section, maybe get rid of the "Example" title and just continue to the text and figure?
| other faces to make connected faces. All the connected faces together make up the grid. | ||
|
|
||
| Example | ||
| ------- |
Member
There was a problem hiding this comment.
Likewise, get rid of the "Example" title and provide the text directly?
Comment on lines
+41
to
+45
|
|
||
| .. image:: ../_static/examples/grids/face_node_edge.png | ||
| :width: 300 | ||
| :align: center | ||
| :alt: A visual of the components of a grid |
Member
There was a problem hiding this comment.
I'd recommend to:
- remove this figure (and corresponding text) from here, and
- use a modification of unstructured_grid.png below where the geometric elements are visualized in the corresponding section in user_guide/terminology
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
PR Checklist
Documentation