[PR]: Add Z axis support for spatial averaging #606

pochedls · 2024-02-14T02:16:36Z

Description

This PR adds functionality to compute vertical averages.

Closes Add Z axis support for spatial averaging #596

Checklist

My code follows the style guidelines of this project
I have performed a self-review of my own code
My changes generate no new warnings
Any dependent changes have been merged and published in downstream modules

If applicable:

I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass with my changes (locally and CI/CD build)
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have noted that this is a breaking change for a major release (fix or feature that would cause existing functionality to not work as expected)

pochedls · 2024-02-14T02:19:18Z

I'd appreciate testing with real-world data (and comparisons with CDAT).

codecov · 2024-02-14T02:19:45Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (5de4d7f) to head (98ea3d7).

Additional details and impacted files

@@            Coverage Diff            @@
##              main      #606   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           15        15           
  Lines         1588      1595    +7     
=========================================
+ Hits          1588      1595    +7

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pochedls · 2024-02-14T15:43:34Z

tests/test_spatial.py

+        xr.testing.assert_allclose(result["so"], expected)
+
+        # check that vertical averaging returns the correct weights
+        expected = xr.DataArray(
+            data=np.array([2000, 2000, 1000, 0.0]),
+            coords={"time": ds.time, "lev": ds.lev, "lat": ds.lat, "lon": ds.lon},
+            dims=["lev"],
+            attrs={"xcdat_bounds": True},
+        )
+
+        xr.testing.assert_allclose(result["lev_wts"], expected)


Are two-in-one tests permitted?

Yeah there's no problem with that here. As long as the tests are relatively easy to maintain.

pochedls · 2024-02-14T15:44:16Z

xcdat/spatial.py

@@ -73,10 +73,12 @@ def average(
        keep_weights: bool = False,
        lat_bounds: Optional[RegionAxisBounds] = None,
        lon_bounds: Optional[RegionAxisBounds] = None,
+        lev_bounds: Optional[RegionAxisBounds] = None,


I think lev_bounds is a good generic name, but I'm open to other possibilities.

Sounds good to me

xcdat/spatial.py

pochedls · 2024-02-14T15:49:01Z

xcdat/spatial.py

+    def _get_vertical_weights(
+        self, domain_bounds: xr.DataArray, region_bounds: Optional[np.ndarray]
+    ) -> xr.DataArray:
+        """Gets weights for the vertical axis.
+
+        This method scales the domain to a region (if selected) and returns weights
+        proportional to the difference between each pair of level bounds.
+
+        Parameters
+        ----------
+        domain_bounds : xr.DataArray
+            The array of bounds for the vertical domain.
+        region_bounds : Optional[np.ndarray]
+            The array of bounds for vertical selection.
+
+        Returns
+        -------
+        xr.DataArray
+            The vertical axis weights.
+        """
+        if region_bounds is not None:
+            domain_bounds = self._scale_domain_to_region(domain_bounds, region_bounds)
+
+        weights = self._calculate_weights(domain_bounds)
+        return weights
+


I think this is sufficiently different from _get_longitude_weights (which deals with the prime meridian) and _get_latitude_weights (sine of bounds) to justify its own function.

tomvothecoder · 2024-02-14T18:21:29Z

Tom:

Compare with cdms2 average with Z axis and get floating point diffs
Try out in e3sm_diags

tomvothecoder · 2024-03-21T17:10:44Z

I'm going to pick this back up for review. We might be able to get this in for v0.7.0.

tomvothecoder · 2024-03-21T17:15:58Z

tests/test_spatial.py

+        so = ds["so"]
+        so[:] = np.array([1, 2, 3, 4])
+        ds["so"] = so


Suggested change

so = ds["so"]

so[:] = np.array([1, 2, 3, 4])

ds["so"] = so

ds["so"].values = np.array([1, 2, 3, 4])

Assigning the numpy array directly to the DataArray will work too

tomvothecoder · 2024-03-21T17:56:41Z

I'd appreciate testing with real-world data (and comparisons with CDAT).

Can you provide me some datasets to test with? I wrote a script and tested a single E3SM dataset, which produced nearly identical results (1e-16 max rel diff).

pochedls · 2024-03-21T23:46:45Z

I'd appreciate testing with real-world data (and comparisons with CDAT).

Can you provide me some datasets to test with? I wrote a script and tested a single E3SM dataset, which produced nearly identical results (1e-16 max rel diff).

Do you have xsearch running? I think you could look for monthly, historical data, for the thetao or ta (Amon table) variables.

tomvothecoder · 2024-03-25T16:06:37Z

I'd appreciate testing with real-world data (and comparisons with CDAT).

Can you provide me some datasets to test with? I wrote a script and tested a single E3SM dataset, which produced nearly identical results (1e-16 max rel diff).

Do you have xsearch running? I think you could look for monthly, historical data, for the thetao or ta (Amon table) variables.

I'll check on xsearch. Thanks for the pointers.

pochedls requested a review from tomvothecoder February 14, 2024 02:16

github-actions bot added the type: enhancement New enhancement request label Feb 14, 2024

pochedls commented Feb 14, 2024

View reviewed changes

tomvothecoder added this to the FY24Q2 (01/01/24 - 03/31/24) milestone Feb 14, 2024

tomvothecoder changed the title ~~Feature/596 vertical averaging~~ [PR]: Add Z axis support for spatial averaging Feb 14, 2024

tomvothecoder modified the milestones: FY24Q2 (01/01/24 - 03/31/24), FY24Q3 (4/01/24 - 06/30/24) Mar 20, 2024

pochedls added 2 commits March 21, 2024 10:13

Initial work on #596

4fa2158

add unit tests for vertical averaging

98ea3d7

tomvothecoder force-pushed the feature/596-vertical-averaging branch from aa9677d to 98ea3d7 Compare March 21, 2024 17:13

tomvothecoder reviewed Mar 21, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PR]: Add Z axis support for spatial averaging #606

[PR]: Add Z axis support for spatial averaging #606

pochedls commented Feb 14, 2024

pochedls commented Feb 14, 2024

codecov bot commented Feb 14, 2024 •

edited

pochedls Feb 14, 2024

tomvothecoder Mar 21, 2024

pochedls Feb 14, 2024

tomvothecoder Mar 21, 2024

pochedls Feb 14, 2024

tomvothecoder Mar 21, 2024

tomvothecoder commented Feb 14, 2024

tomvothecoder commented Mar 21, 2024

tomvothecoder Mar 21, 2024 •

edited

tomvothecoder commented Mar 21, 2024

pochedls commented Mar 21, 2024

tomvothecoder commented Mar 25, 2024

[PR]: Add Z axis support for spatial averaging #606

Are you sure you want to change the base?

[PR]: Add Z axis support for spatial averaging #606

Conversation

pochedls commented Feb 14, 2024

Description

Checklist

pochedls commented Feb 14, 2024

codecov bot commented Feb 14, 2024 • edited

Codecov Report

pochedls Feb 14, 2024

Choose a reason for hiding this comment

tomvothecoder Mar 21, 2024

Choose a reason for hiding this comment

pochedls Feb 14, 2024

Choose a reason for hiding this comment

tomvothecoder Mar 21, 2024

Choose a reason for hiding this comment

pochedls Feb 14, 2024

Choose a reason for hiding this comment

tomvothecoder Mar 21, 2024

Choose a reason for hiding this comment

tomvothecoder commented Feb 14, 2024

tomvothecoder commented Mar 21, 2024

tomvothecoder Mar 21, 2024 • edited

Choose a reason for hiding this comment

tomvothecoder commented Mar 21, 2024

pochedls commented Mar 21, 2024

tomvothecoder commented Mar 25, 2024

codecov bot commented Feb 14, 2024 •

edited

tomvothecoder Mar 21, 2024 •

edited