Skip to content
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

Merged
merged 2 commits into from
Aug 27, 2024

Conversation

owenlittlejohns
Copy link
Member

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.
  • Documentation updated (if needed).

Copy link
Member

@flamingbear flamingbear left a 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.

@@ -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",
Copy link
Member

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?

Copy link
Member Author

@owenlittlejohns owenlittlejohns Aug 26, 2024

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 or VarInfoFromNetCDF4 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 as VarInfoFromNetCDF4.get_required_variables. The metadata attributes expected to contain references to other variables, per the CF Conventions include:

Copy link
Member

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.

Copy link
Member Author

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

"* [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",
Copy link
Member

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

Copy link
Member Author

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.

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member Author

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.

@owenlittlejohns owenlittlejohns merged commit fdb7260 into main Aug 27, 2024
6 checks passed
@owenlittlejohns owenlittlejohns deleted the update-documentation branch August 27, 2024 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants