Adding beginner friendly explanations on the Docs - #1607
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
left a comment
Sevans711
left a comment
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?
|
@Sevans711 Good notes! And thank you for the through read!
|
|
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
left a comment
erogluorhan
left a comment
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 | ||
| ================================ |
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 | ||
|
|
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?
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
Sevans711
left a comment
Sevans711
left a comment
There was a problem hiding this comment.
Looks like great work so far!
I have some suggestions / comments on the new version. There are a decent number of them but I think most of them are minor. All lingering feedback has been left as inline comments rather than in this message, in case that makes things easier to track.
Checking points from my original review:
- (done!) I see that iso_grid.png file is <1MB now.
- (done!) I don't see "grid points" in the new version anymore.
- (done/followed up with inline comment) The new unstructured_grid.png diagram looks improved compared to the old one.
- (done!) The new unstructured_grid.png diagram contains both hexagon and non-hexagon faces.
- (done/in progress?) The expanded example descriptions have made links less necessary. Still could add links to relevant publications if available, but okay to move forward without them.
- (done!) Changed spelling to "UxDataArray" instead of "Data Array" in UxDataset description, and explained in PR comment threads the reason for not editing UxDataArray description as suggestion.
- (not done) Colors are still unchanged. I left an inline comment in this review, to help track this issue more easily.
- (done) The original issue description and the PR descriptions have both been updated to clearly show how each part of the issue has been solved.
There was a problem hiding this comment.
The numbers are extremely low resolution here; if keeping this file consider a slightly higher-res version of it!
There was a problem hiding this comment.
The description says "you may end up in a hole" but there are no "holes" in this image, just regions with faces and regions without faces. I think of a hole as "an empty region surrounded by non-empty regions". Maybe add a few more differently-shaped faces connecting the bottom right cell to the second-row left cell?
Additionally, maybe talking about this grid would be easier if you label the faces with letters? E.g., "move one step right from face A and you end up on face B, but move another step right from face B and you fall off the grid entirely" or something like that?
| "1. **[`uxarray.Grid`](https://uxarray.readthedocs.io/en/latest/generated/uxarray.UxDataArray.html)**: Stores the grid representation (i.e. coordinates, connectivity information, etc.)\n", | ||
| "2. **[`uxarray.UxDataset`](https://uxarray.readthedocs.io/en/latest/api.html#uxdataset)**: One or more data variable that resided on a grid.\n", | ||
| "3. **[`uxarray.UxDataArray`](https://uxarray.readthedocs.io/en/latest/api.html#uxdataarray)**: A single data variable that resides on a grid\n" | ||
| "2. **[`uxarray.UxDataset`](https://uxarray.readthedocs.io/en/latest/api.html#uxdataset)**: One or more data variables that resided on a grid. Each variable is stored in a UxDataArray.\n", |
There was a problem hiding this comment.
resided --> reside; they still live on the grid.
There was a problem hiding this comment.
Consider slightly larger dots for nodes, and color code a few nodes and edges. E.g., put nodes in blue and the word "node" in blue, and the pointed-to edge in red and the word "edge" in red
| :align: center | ||
| :alt: A structured grid with a regular, matrix-like arrangement of cells | ||
|
|
||
| Consider the cell bounded by 19°E–20°E and 19°N–20°N. Its four neighboring cells are bounded by: |
There was a problem hiding this comment.
Recommendation: use half-degrees for nodes in this example, so that faces are centered at whole-degree values, and then refer to faces by their center (similar to what you had in an earlier version). Then, the text can say something like "Consider the cell centered (20°E, 20°N). Its four neighboring cells are at (north) (20°E, 21°N), (south) (20°E, 19°N), …". As-is, this section is too verbose to parse quickly (I agree with your thoughts on this from our conversation earlier).
I do think it is okay to stick with basically the image format here though, rather than reverting to your older 5-cell image. If you want to improve quick-glance + skimming-the-text value maybe give slight colors to some of the cells to reference in the text as well? E.g. "(north/light blue) (20°E, 21°N), (south/light green) (20°E, 19°N), …"
| cells on land. They can use a model that has no cells on land, | ||
| which creates large holes that structured grids could not run calculations on. | ||
|
|
||
| In a traditional lat/lon grid, ~30% of the grid cells in the previous example would be |
There was a problem hiding this comment.
Suggestion: add "oceans-only" or some other concise phrase to describe the example, not just "previous example". I was skimming at first and it took me a bit too long to understand what this meant originally.
|
|
||
| In a traditional lat/lon grid, ~30% of the grid cells in the previous example would be | ||
| unused, meaning the data footprint could be roughly 30% smaller. Many | ||
| calculations will likely require noticeably fewer resources, and less time will be required to |
There was a problem hiding this comment.
Suggestion: "…less time will be required to compute" --> "… reducing compute time and dataset sizes."
| unused, meaning the data footprint could be roughly 30% smaller. Many | ||
| calculations will likely require noticeably fewer resources, and less time will be required to | ||
| compute. Many nodes, edges, and faces are eliminated by the removal of the | ||
| land, and even the cells over the sea have fewer connected faces, nodes, |
There was a problem hiding this comment.
"the cells over the sea have fewer connected faces, nodes, and edges as a result" is a good reason to remove the land, but I was confused at first because the rest of this page seems to be all about comparing structured & unstructured. This statement is not related to structured vs unstructured. I think it would be okay to remove; the previous sentences already clarify some structured vs unstructured benefits here.
| ------------ | ||
|
|
||
| These user guides provide detailed explanations of the core functionality in UXarray. | ||
| These user guides provide detailed explanations of the core functionality in UXarray |
There was a problem hiding this comment.
Restore the period at end of sentence
| <style> | ||
| .yes-cell { | ||
| background-color: green; | ||
| background-color: #6aa84f; /* Light green color */ |
There was a problem hiding this comment.
Did not change actual rendering of table, as far as I can tell. Either revert the color changes to this file, or debug and fix so that the new colors are actually used. (I think light green and light red would be an improvement, if you do manage to get them to work!)
erogluorhan
left a comment
erogluorhan
left a comment
There was a problem hiding this comment.
Getting there, please see some more comments below
| @@ -0,0 +1,111 @@ | |||
| .. currentmodule:: uxarray | |||
There was a problem hiding this comment.
All the figures except ocean.png renders too large in this document, please see https://uxarray--1607.org.readthedocs.build/en/1607/user-guide/unstructured-grids.html
Just a clarification if this was led by my previous comment: I only meant ocean.png was rendering too small previously, not the other(s)
|
|
||
| Before diving into unstructured grids, it is helpful to understand the basic differences | ||
| between structured and unstructured grids. Unstructured grids differ from structured grids | ||
| in how they are designed, navigated, and used in modeling functions. |
There was a problem hiding this comment.
Can avoid some redundant wording here by combining the last two sentences, and also did you mean "model analysis" instead of "models":
"...understand the basic differences between structured and unstructured grids in how they are designed, navigated, and used in models."
|
One higher-level direction that may help simplify the remaining edits: rather than making this a broad, standalone primer on all structured-versus-unstructured-grid tradeoffs, could we center it on the question a new UXarray user has: what is different about an unstructured grid, and what does UXarray expose to work with it? I suggest keeping the visual comparison intentionally short, then following it with a small “Working with an unstructured grid in UXarray” section that:
The current detailed discussion of polar singularities, land masking, model specialization, and performance is useful context, but it is hard to state generally without qualifications and overlaps with the cookbook material. It may be better as a concise “Why models use them” callout with links, or kept in the cookbook. This would give the page a clearer UXarray-specific learning path while reducing the amount of broad background that needs continued refinement. |
Basically two suggestions that could help focus this page: |
|
I don't really see the need for an explainer on structured vs. unstructured grid within the UXarray docs. Who is this for? Anyone landing on the UXarray page already has unstructured grid data and is looking for a python library to work with it. We only need to explain what subsets of unstructured grids we handle (which is done in #1626) and high level usage differences with Xarray. |
Yeah that makes sense, given UXarray is already targeting this domain. How about something like this:
cc: @rajeeja to discuss it with your high level suggestions |
5 participants
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