DAS-2368: Configure & support dimension variables for SMAP L3 at the root level by joeyschultz · Pull Request #36 · nasa/harmony-metadata-annotator

joeyschultz · 2025-11-11T14:54:57Z

Description

This PR adds the ability to create up-level (shared) dimensions.
The varinfo configuration was modified to create shared dimensions at the root level for:

SPL3FTA
SPL3SMA
SPL3SMP
SPL3SMP_E

Jira Issue ID

DAS-2368

Local Test Steps

Check out this branch and build the new image and run the tests

./bin/build-image && ./bin/build-test && ./bin/run-test

Ensure your HIAB configuration has:

LOCALLY_DEPLOYED_SERVICES=harmony-metadata-annotator,hoss,sds-maskfill

Restart harmony to pick up the new image:

./bin/bootstrap-harmony

Run the SPL3SMP_E bbox request in UAT to see the current behavior (dimensions created in each group):
https://harmony.uat.earthdata.nasa.gov/C1268399578-EEDTEST/ogc-api-coverages/1.0.0/collections/parameter_vars/coverage/rangeset?forceAsync=true&subset=lat(60%3A85)&subset=lon(-70%3A-10)&label=harmony-py&maxResults=1&variable=Soil_Moisture_Retrieval_Data_AM%2Fsurface_flag&variable=Soil_Moisture_Retrieval_Data_PM%2Fsurface_flag_pm&variable=Soil_Moisture_Retrieval_Data_Polar_AM%2Fsurface_flag&variable=Soil_Moisture_Retrieval_Data_Polar_PM%2Fsurface_flag_pm

Now run the same request on localhost to see the change (shared dimensions created up-level):
http://localhost:3000/C1268399578-EEDTEST/ogc-api-coverages/1.0.0/collections/parameter_vars/coverage/rangeset?forceAsync=true&subset=lat(60%3A85)&subset=lon(-70%3A-10)&label=harmony-py&maxResults=1&variable=Soil_Moisture_Retrieval_Data_AM%2Fsurface_flag&variable=Soil_Moisture_Retrieval_Data_PM%2Fsurface_flag_pm&variable=Soil_Moisture_Retrieval_Data_Polar_AM%2Fsurface_flag&variable=Soil_Moisture_Retrieval_Data_Polar_PM%2Fsurface_flag_pm

Accompanying regression test PRs:

das-harmony-regression-tests: PR#68
harmony-regression tests: TBD

PR Acceptance Checklist

Jira ticket acceptance criteria met.
CHANGELOG.md updated to include high level summary of PR changes.
docker/service_version.txt updated if publishing a release.
Fix version [harmony-metadata-annotator-X.Y.Z] added to Jira ticket if publishing a release.
Tests added/updated and passing.
Documentation updated (if needed).

…ensions

joeyschultz · 2025-11-11T15:07:31Z

metadata_annotator/history_functions.py

    }

-    return var_dim_map
+    # Identify dimensions that have been created up-level and update map


As the docstring notes, this code for updating the var_dim_map is only temporary required until earthdata-varinfo supports up-level dimensions.

flamingbear

This is mostly questions for you to educate me. I have a couple of suggestions. But it's very close, I did run the test requests and looked at the outputs. Nice.

metadata_annotator/earthdata_varinfo_config.json

flamingbear · 2025-11-12T22:55:20Z

metadata_annotator/earthdata_varinfo_config.json

        "Mission": "SMAP",
        "ShortNamePath": "SPL3SMP$",
-        "VariablePattern": "/Soil_Moisture_Retrieval_Data_(AM|PM)/(?!lc_type$).+$"
+        "VariablePattern": "/Soil_Moisture_Retrieval_Data_(AM|PM)/.*"


~~and no lc_types? (are these gone now from exclusions?)~~ looks like it's hoisted?

See my explanation for why I removed the am_pm exclusion from the variable pattern in the above comment. The same applies here - just for lc_type in this case

flamingbear · 2025-11-12T23:15:54Z

metadata_annotator/annotate.py

+            for parent in node.parents:
+                dim_candidate_path = construct_dim_path(parent.path, dim)
+                if dim_candidate_path in variables_to_create:
+                    parent.ds = parent.ds.assign(**{dim: dim_src})


do you need to unpack that dict? couldn't you just assign it as a dict?

Suggested change

parent.ds = parent.ds.assign(**{dim: dim_src})

parent.ds = parent.ds.assign({dim: dim_src})

Nice catch. Updated in ae7d2bf.

flamingbear · 2025-11-12T23:24:06Z

metadata_annotator/annotate.py

+        for node in datatree.subtree
+        for dim in node.ds.dims
+        if (dim_full_path := construct_dim_path(node.path, dim)) in variables_to_create
+    }


I can't tell if this is clever or I'm just dense because I didn't know the walrus operator. I'm almost tempted to say it should just be nested loops for readability, but maybe I'm off.

Discussed with Matt and decided to use nested for loop to simplify things. I had tried making the set comprehension work but I don't think its worth the complexity.

Updated in ae7d2bf.

flamingbear · 2025-11-12T23:47:19Z

metadata_annotator/history_functions.py

    }

-    return var_dim_map
+    # Identify dimensions that have been created up-level and update map


I'm looking at the original and I don't understand what it does or how it's used.
It looks like it just get all of the dimension pairs, but then truncates all but one value from the list of variables.

L139-L142. I'm baffled why we would do that. (git blame says it was me, but the ticket link is to some epic)

flamingbear · 2025-11-12T23:51:28Z

metadata_annotator/history_functions.py

+                    f'{parent.path}/{dimension_name}'
+                    if parent.path != '/'
+                    else f'/{dimension_name}'


Didn't you have a helper for this somewhere?

Discussed with Matt and landed on using an inner helper function for this case. I'm unable to import annotate::construct_dim_path due to circular imports and don't think its worth creating a new module to get around that because this invocation of construct_dim_path will go away once the varinfo update is made.

Updated in d17535f.

flamingbear · 2025-11-12T23:52:29Z

tests/data/earthdata_varinfo_test_config.json

+          "Value": "y X"
+        }
+      ],
+      "_Description": ""


Either add a description or delete this, I think.

Updated in da31df7.

In this commit, I also made some updates to the function that uses this test varinfo config entry (test_update_dimension_variables) to properly test that we are renaming the dimensions. I also moved the text fixture datatree and varinfo object directly into the test because this was the only test using them.

flamingbear · 2025-11-12T23:58:15Z

tests/unit/test_annotate.py

-        assert set(
-            datatree['/Freeze_Thaw_Retrieval_Data_Global/surface_flag'].dims
-        ) == set(['am_pm', 'y', 'x'])
-


~~are these at the top level now? could they be checked there?~~ doesn't look like it.

This assertion of the renaming was deleted because this PR removes the dimension renaming functionality from update_group_and_variable_attributes.

flamingbear · 2025-11-13T00:08:26Z

tests/unit/test_annotate.py

+)
+def test_has_dimension_override(sample_varinfo_test06, variable_path, expected):
+    """Ensure variables correctly evaluated to contain non-null dimensions override."""
+    assert has_dimension_override(sample_varinfo_test06, variable_path) == expected


This is a lot of indirection to get to what this test does. I don't have a suggestion, mostly complaints.

To make the test easier to understand, I moved the test datatree and varinfo object directly into the test function. These test06 fixtures were not used by any other tests so I think this change to creating them directly in the test function is better.

Updated in da31df7

flamingbear · 2025-11-13T00:19:33Z

tests/unit/test_annotate.py

+    with xr.open_datatree(sample_netcdf4_file_test07) as test_datatree:
+        assert (
+            get_new_dimension_variables(test_datatree, variables_to_create) == expected
+        )


I don't really understand this test.

It might help if there was a test that asked for more variables to create than exist on the file. because that passes too.

I'm not even sure if that' useful information or if that would ever happen
e.g.

( {'/y_root', '/sub_group/y', '/missing/dimension/z'}, {'/y_root', '/sub_group/y'}, ),

I agree that the suggested test case is more useful than what I previously had.

Updated in da31df7

flamingbear · 2025-11-17T17:58:11Z

tests/data/earthdata_varinfo_test_config.json

        {
          "Name": "dimensions",
-          "Value": "y X"
+          "Value": "y x"


flamingbear

Thanks for making all those changes. This looks good. Hopefully we can yank some of it out soon.

joeyschultz added 5 commits November 7, 2025 11:22

DAS-2368: Create SMAP L3 shared dimension variables at root level

1738775

DAS-2368: Update varinfo config

df159e4

DAS-2368: Combine SPL3SMA overrides with the same variable pattern

cf2ac17

DAS-2368: Combine SPL3SMP_E overrides with the same variable pattern

1644517

DAS-2368: Modify get_variable_dimension_map to support up-level dim…

8482314

…ensions

joeyschultz commented Nov 11, 2025

View reviewed changes

flamingbear reviewed Nov 13, 2025

View reviewed changes

joeyschultz added 3 commits November 13, 2025 14:50

DAS-2368: Add inner helper function to enhance readability

d17535f

DAS-2368: Simplify code logic for clarity

ae7d2bf

DAS-2368: Update tests to address PR feedback

da31df7

joeyschultz mentioned this pull request Nov 13, 2025

DAS-2368: Add configuration for SMAP L3 to support root-level dimensions nasa/harmony-regridding-service#53

Merged

6 tasks

DAS-2368: Fix incorrect attribute on SPL3SMP_E dimension variables

529e5c8

flamingbear reviewed Nov 17, 2025

View reviewed changes

tests/data/earthdata_varinfo_test_config.json

{

"Name": "dimensions",

"Value": "y X"

"Value": "y x"

Copy link

Member

flamingbear Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noice

flamingbear approved these changes Nov 17, 2025

View reviewed changes

joeyschultz merged commit 1915616 into main Nov 17, 2025
4 checks passed

joeyschultz deleted the DAS-2368 branch November 17, 2025 19:03

	parent.ds = parent.ds.assign(**{dim: dim_src})
	parent.ds = parent.ds.assign({dim: dim_src})

Conversation

joeyschultz commented Nov 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Jira Issue ID

Local Test Steps

PR Acceptance Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flamingbear left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flamingbear left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

joeyschultz commented Nov 11, 2025 •

edited

Loading