-
Notifications
You must be signed in to change notification settings - Fork 3
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
TRT-290 - Augment documentation notebook regarding required variables. #35
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
checked all the links, they go where I would expect. take or leave my questions/advice.
docs/earthdata-varinfo.ipynb
Outdated
@@ -198,6 +198,19 @@ | |||
"source": [ | |||
"In the output above, `/Grid/precipiationCal` has three dimensions: `/Grid/time`, `/Grid/lat` and `/Grid/lon`. These dimension variables also refer to their respective [bounds variables](http://cfconventions.org/Data/cf-conventions/cf-conventions-1.10/cf-conventions.html#cell-boundaries). Because `get_required_variables` is recursive, the bounds attributes are also considered required variables for `/Grid/precipitationCal`.\n", | |||
"\n", | |||
"`earthdata-varinfo` checks for references in known CF Convention metadata attributes that are expected to contain references to other variables. These include:\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it earthdata-varinfo checking for references or this particular call to get_required_variables
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good question. Here's the long answer:
The VarInfo*
(or perhaps more specifically: Variable*
) classes will try to identify all the references when a file is initially parsed. But all that means is checking the expected metadata attributes and then trying to fully qualify the references it finds based on the location of the variable making that reference and the value of the metadata attribute. (That is - there is no checking that the variable being referred to exists, and there is no recursive tracing at that point)
VarInfoBase.get_required_variables
is where the dependencies are traced to get a full set.
So maybe what I'm saying is it's a little of column A and a little of column B.
TL;DR: Maybe a more accurate update would be:
When a
VarInfoFromDmr
orVarInfoFromNetCDF4
object is instantiated, specific CF Convention metadata attributes of each variable are checked for references to other variables. These references are then fully qualified to absolute paths for later use by methods such asVarInfoFromNetCDF4.get_required_variables
. The metadata attributes expected to contain references to other variables, per the CF Conventions include:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this more accurate update is better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool. I have added that here: b50c3d
docs/earthdata-varinfo.ipynb
Outdated
"* [cell_measures](https://cfconventions.org/Data/cf-conventions/cf-conventions-1.11/cf-conventions.html#cell-measures)\n", | ||
"* [geometry, interior_ring, node_coordinates, node_count, nodes and part_node_count](https://cfconventions.org/Data/cf-conventions/cf-conventions-1.11/cf-conventions.html#geometries)\n", | ||
"\n", | ||
"The check is recursive, as a variable referred to in the initial set specified in the call to the `get_required_variables` method might itself refer to other variables.\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You do mention it's recursive right above this change also, I don't think it's a big deal, but wasn't sure if you wanted to reword a bit? Maybe Because the check is recursive,
would be enough
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll make that change and push it up with the potential other change being discussed, once we settle on wording there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at it, and the content from before, is this sentence actually adding anything? Could we just cut it in favour of the comment above about how the bounds
variables are included.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know that was my first thought but then again people skim documentation so repeating isn't terrible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't think of a good way to make two entirely distinct points here, so I've cut it in b50c3d.
Description
This PR updates the main
earthdata-varinfo
documentation Jupyter notebook to clarify the CF Convention metadata attributes that are used to determine "required variables".Jira Issue ID
TRT-290 (this request came up in the context of supporting SMAP migration to the cloud, but the Jira issue is a bit tangential).
Local Test Steps
N/A
PR Acceptance Checklist
Jira ticket acceptance criteria met.CHANGELOG.md
updated to include high level summary of PR changes.VERSION
updated if publishing a release.Tests added/updated and passing.