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?
| :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.
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 |
Removing existing notes and redirecting to project pythia instead
|
@rljacob @rajeeja The goal of this was to target a different user workflow. Like from my experience, I was educated as a developer, but work within domains that I wasn't formally educated in, like transit systems or meteorology. I have been assigned to incorporate many geospatial tools, packages, etc without my employer training me in said tools. I know the struggle of being skilled in a field, but having to apply it to a new domain with little knowledge about said domain, while in a company that can't/won't teach or train you. The docs are the first place a developer with a desire for knowledge will go, and I thought it may benefit us to sprinkle bits of education along the path a new user would take, so they can understand the package better, share it with others in words they understand, and feel more confident incorporating it into their workflow. Because this content is so specific and niche, there aren't many other places to learn this content from. I understand this doesn't benefit the core users, but the core users likely aren't on pages like From the follow up conversations, it seems like this kind of new user content is sufficiently explained on the Pythia site and many resources can be found there, so I've added a redirect there where @erogluorhan and I discussed separately. For now the content there still seems heavily tailored to users already familiar with geospatial language. I have removed the Structured vs Unstructured Grids content, and replaced old connections in favor of redirects to Project Pythia. |
|
Thank you for all your work on this, @dylannelson, even as you ended up removing a big chunk of it from this PR. I see you clarified some of the changes above. I would be happy to review again, but first a few bookkeeping questions/thoughts:
While answering these questions please feel free to point to your updated original message in this PR (e.g. maybe it already answers (1) and (2), but I just wanted to clarify before reviewing). |
|
@Sevans711 Sorry I'm a bit confused about what you mean. It's mostly just the two points mentioned in the description. Content was added to the Tutorials/Videos page, made a slight tweak to a definition in a notebook, and two other small changes not mentioned: changing the colors in a table, and making some image sizes smaller. Did you want this info placed somewhere else too? The original page that was planned has been removed and is now crossed out in the description. Just scrapping it for now as it seemed to not be needed. I just crossed it out in the issue as well but if you think it needs a comment also discussing it there, I can do that as well, but why would it be needed when these two discussions are linked? |
|
No worries, I am happy to try to clarify further. Please let me know if this helps! With (1) & (2), I was asking them because the scope of the PR changed. I can see the changes that have been made, but I am hoping for more clarity on what exactly is the goal of this PR now, such as a comprehensive list of tasks it intends to complete. When reviewing, I want to be able to answer the question "has this PR actually accomplished everything it claimed to accomplish?" To answer (2), it would be sufficient to put a list in a comment in this thread. E.g.: "Here are all the changes this PR intends to make: (i) reduce file size for some images, (ii) soften colors on grid comparison table, (iii) add links to tutorial videos on the tutorials.rst page, (iv) …" To answer (1), it would be sufficient to say something like "All other initially-proposed changes are no longer intended to be part of this PR, as per discussion above." For (3), I have been thinking of issue reports and PRs as distinct entities. The issue report should provide a set of desired changes and a reason for wanting those changes. In this case (#1582), the list of desired changes was made explicit, while the reason remains implicit. Either way is fine, as long as they can be inferred (e.g., it is clear here that the reason is something like "the docs should be more beginner-friendly"). When the scope of the PR changes due to discussion in the PR thread (like what happened here), that signals to me one of the following:
When (A) happens, the issue is just with the PR, and it makes sense to fix it just within the PR, without touching the original issue. However, (B) and (C) indicate something is "wrong" or unexpected with the original issue, and a shift in goals from "solve this as written" to "solve only part of this" or "solve it differently than how it was written". My question (3) was leaning into this notion; to me it makes the most sense to leave a comment on the initial issue clarifying that (B) or (C) occurred, and explaining why it occurred (even if the explanation is just "see discussion in PR for details"). Without any comments to indicate otherwise, when I'm looking through closed issues my default assumption is that each one has been fully implemented as described/implied by the issue report, for the reasons described/implied by the report. |
The original page that was planned has been removed and is now crossed out in the description. Just scrapping it for now as it seemed to not be needed, as per the discussion above.
The priorities and direction changed, as per the discussion above
Yes, it may be going to Pythia in some capacity I hope that answers the questions above |
Sevans711
left a comment
Sevans711
left a comment
There was a problem hiding this comment.
Looks good to me! Confirmed all now-intended changes have been incorporated and look good, as per @dylannelson's last comment:
- Updating Tutorials and Videos Page
- (done!) Added content to
docs/tutorials.rst - (done!) Added 2 new links to 2 videos from 2024
- (done!) Added brief context for each of the 3 links now on the page
- More distinct defintions for UxDataAray vs UxDataset
- (done!) Defined here: user-guide/data-structures
- (done) Changed slightly after gathering feedback from team
- Other Fixes
- (done!) Changes colors of a table
- (done!) Reduced image file size
Thank you also for leaving a comment on the original issue. I think it is now safe to say this PR fully closes the clarified-to-have-reduced-scope issue.
Yes, it may be going to Pythia in some capacity
That would be great! I appreciated your writeups and that seems like a good way to keep these efforts and helpful materials from getting lost. (But, this can be a separate issue at a later time. This PR seems ready to merge as-is.)
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 gridsCreated a whole new page, now inuxarray\docs\user-guide\unstructured-grids.rstCreated or located 5 new imagesAdded connections so this page appears in other lists and taskbars2) Updating Tutorials and Videos Page
docs/tutorials.rst3) More distinct defintions for UxDataAray vs UxDataset
PR Checklist
Documentation