DAS-2236 - Updates to methods that calculate the points used to create the dimensions #18

sudha-murthy · 2024-11-14T15:16:00Z

Description

functions added to be able to do end toe nd test
updated functions that calculated the two sets lat/lon points to calculate the dimension arrays
A couple of unit tests need to be added
This does not support any test for ordering, multiple grids or 3D support

Jira Issue ID

DAS-2236

Local Test Steps

Make sure the unit tests pass
if you can test with harmony in a box with mask fill disabled - you can test the following url

http://localhost:3000/C1268452378-EEDTEST/ogc-api-coverages/1.0.0/collections/parameter_vars/coverage/rangeset?forceAsync=true&granuleId=G1268452388-EEDTEST&subset=lat(70%3A80)&subset=lon(-160%3A-150)&variable=Soil_Moisture_Retrieval_Data_AM%2Fstatic_water_body_fraction&skipPreview=true
and verify you can check the outputs in panoply

PR Acceptance Checklist

[X ] 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.
[X ] Tests added/updated and passing.
Documentation updated (if needed).

…mum distances

hoss/coordinate_utilities.py

tests/unit/test_spatial.py

hoss/coordinate_utilities.py

hoss/dimension_utilities.py

owenlittlejohns

I took a look, and I appreciate this PR being another more bite-sized piece of progress towards the end goal.

I think there are few themes to my comments:

You haven't added sufficient unit tests to provide thorough coverage of your changes. In most cases, you've got some tests for new functions, but not all. The tests you've added are generally good, but often are only testing the happy path. It's pretty important to also test realistic edge-cases, which isn't really done in this PR.
There are quite a lot of places where you are performing very similar tasks, but in some cases along a row, rather than up-and-down a column. Some of those tasks are basically the same thing (performing some operation on a 1-D slice of an array), so it would be cleaner to write a function to do something to a 1-D slice and then invoke it with the correct 1-D slice for either a row or a column.
I think generally, there are a few places where the documentation strings for each function could do with a bit more descriptive text. This will be the number one location that future developers try to read to understand what the code is meant to do.
General coding comments.

owenlittlejohns · 2024-11-18T15:41:45Z

hoss/coordinate_utilities.py

@@ -3,16 +3,16 @@
    coordinate variable data to projected x/y dimension values
 """

+from typing import Dict


Nit: I think the better way to go is just to use the Python built-in dict, much as you already do for list and tuple. That means you don't need to import typing.Dict here. (When we don't have a big feature on the boil for HOSS, I'm going to go through and clean up the other modules that currently use typing.Dict, typing.List, etc, for consistency)

updated. - 1c29221

owenlittlejohns · 2024-11-18T15:44:02Z

hoss/coordinate_utilities.py

@@ -101,20 +101,23 @@ def get_coordinate_variables(
    CF-Convention coordinates metadata attribute. It returns them in a specific
    order [latitude, longitude]"
    """
-
-    coordinate_variables_list = varinfo.get_references_for_attribute(
+    # varinfo returns a set and not a list


Do you really need this comment? It seems a bit superfluous.

updated - 1c29221

owenlittlejohns · 2024-11-18T15:46:24Z

hoss/coordinate_utilities.py

@@ -101,20 +101,23 @@ def get_coordinate_variables(
    CF-Convention coordinates metadata attribute. It returns them in a specific
    order [latitude, longitude]"


I know this is a comment on the previously written documentation string, but this description strongly implies the return of this function is a list containing a single latitude and longitude variable. Instead it returns a list with two sub-lists (one each of latitude and longitude variable names).

Also,

This function returns latitude and longitude variables...

probably would be clearer as:

This function returns latitude and longitude variable names...

(E.g., it specifically returns the names of the variables, not the variables themselves)

done - 1c29221

owenlittlejohns · 2024-11-18T15:50:50Z

hoss/coordinate_utilities.py

-        for coordinate in coordinate_variables_list
-        if varinfo.get_variable(coordinate).is_latitude()
+        for coordinate in coordinate_variables
+        if varinfo.get_variable(coordinate) is not None


(Same comment for the similar change in the list comprehension for longitude_coordinate_variables)

Where do you catch any issues if there is a reference to a variable in a coordinates metadata attribute that isn't present in the file? If it's pointing to a latitude or longitude variable that isn't present in the file, at some point that's going to cause a failure when you try to access that variable. Is that gracefully handled somewhere?

(It's likely this is already nicely supported, but it's been a while since the previous PR and I just can't remember)

Separate comment - you have not added unit tests to prove this case is being handled as expected. (Needed both for latitude or longitude)

this method is first called before prefetch. it should not fail if the variables don't exist.

if it is present in the variable and not in the file - it will fail during prefetch. That is when it is first accessed from the file. - will check if I have a test on that

There is a unit test for the function - will add subtests if the variables don't exist

There are unit tests to check for the above cases in line 205 of test_coordinate_utilities.py
https://github.com/nasa/harmony-opendap-subsetter/blob/57744a6298adf081d0bf06c9a6acd0b98cccf99c/tests/unit/test_coordinate_utilities.py#L205C1-L219C1

hoss/coordinate_utilities.py

owenlittlejohns · 2024-11-18T20:20:06Z

hoss/coordinate_utilities.py

+    x_y_points = list(zip(points_x, points_y))
+
+    return x_y_points


You can avoid declaring a variable for a single immediate use and just do:

return list(zip(points_x, points))

done. - 1c29221

owenlittlejohns · 2024-11-18T20:20:58Z

hoss/coordinate_utilities.py

+    points: list[tuple],  # list of data points as tuple in (lon, lat) order
+    crs: CRS,  # CRS object from PyProj
+) -> list[tuple]:  # list of X-Y points as tuple in (X, Y) order


This is another place where the in-line comments should go in the documentation string.

done - 2f07d1a

owenlittlejohns · 2024-11-18T20:25:53Z

tests/unit/test_coordinate_utilities.py

+        """Ensure that two valid lat/lon points returned by the method
+        with a set of lat/lon coordinates as input
+
+        """


Nice tests. I think there are a couple of additional missing test-cases, though:

There is only a single valid pixel.

There are no valid pixels.

Possibly also what happens when there are only valid points in a single row or single column (for geographic things).

updated unit tests - a3680c5

owenlittlejohns · 2024-11-18T20:31:33Z

tests/unit/test_coordinate_utilities.py

+        returned by the method with a set of lat/lon coordinates as input
+
+        """
+        with self.subTest('Get two sets of valid indices from coordinates dataset'):


Similar comments to a couple of other places. This unit test is only really testing the happy-path. You aren't really throwing any potential edge-cases at the function. Other test cases are:

Only a single valid pixel.

Only valid pixels in a single row.

Only valid pixels in a single column.

No valid pixels.

These seem like overkill, but it's really important to know how this code behaves in realistic edge-cases, so we know failure is graceful.

updated unit tests - 57744a6

owenlittlejohns · 2024-11-18T20:47:19Z

tests/unit/test_dimension_utilities.py

@@ -328,7 +328,7 @@ def test_get_fill_slice(self):

    @patch('hoss.dimension_utilities.add_bounds_variables')
    @patch('hoss.dimension_utilities.get_opendap_nc4')
-    def test_prefetch_dimension_variables(
+    def test_get_prefetch_variables(


Given that you've added conditional logic to the get_prefetch_variables function, you need to add more test-cases for it. The get_prefetch_variables function will need a subtest (or even separate tests if you prefer) that runs through each branch of code.

Right now, I think this test just ensures variables with dimensions, but not bounds, behave nicely. There's also test_prefetch_dimensions_with_bounds lower down in the file that ensures variables with dimensions and bounds work well. But there's nothing invoking prefetch_variables for requested variables with anonymous dimensions.

updated - 3b0a149

D-Auty · 2024-11-22T20:04:35Z

I'll check in after Owen has had a chance to review. I've reviewed my suggestions and updates made are ok.

sudha-murthy added 3 commits November 6, 2024 14:47

DAS-2236 - added functions to get end to end test working

1ad0173

DAS-2232 - updated the get_valid_geo_grid_points function to get maxi…

2fb332d

…mum distances

DAS-2236 - updated the method to get the points

51c3928

sudha-murthy requested review from owenlittlejohns and D-Auty November 14, 2024 15:16

sudha-murthy added 3 commits November 14, 2024 15:34

DAS-2236 - Added missing unit tests

089f6a5

tests/unit/test_coordinate_utilities.py

2a7e61e

DAS-2236 - refactoring and cleanup

5b9105c

autydp reviewed Nov 15, 2024

View reviewed changes

hoss/coordinate_utilities.py Show resolved Hide resolved

hoss/coordinate_utilities.py Show resolved Hide resolved

hoss/coordinate_utilities.py Show resolved Hide resolved

hoss/coordinate_utilities.py Show resolved Hide resolved

hoss/coordinate_utilities.py Outdated Show resolved Hide resolved

DAS-2236 - updates based on PR feedback

f0e283a

autydp reviewed Nov 15, 2024

View reviewed changes

hoss/coordinate_utilities.py Outdated Show resolved Hide resolved

DAS-2236 - comment update based on PR feedback

0d6c01b

autydp reviewed Nov 16, 2024

View reviewed changes

tests/unit/test_spatial.py Show resolved Hide resolved

autydp reviewed Nov 17, 2024

View reviewed changes

D-Auty reviewed Nov 17, 2024

View reviewed changes

hoss/coordinate_utilities.py Outdated Show resolved Hide resolved

D-Auty reviewed Nov 18, 2024

View reviewed changes

hoss/dimension_utilities.py Outdated Show resolved Hide resolved

hoss/dimension_utilities.py Show resolved Hide resolved

hoss/dimension_utilities.py Show resolved Hide resolved

owenlittlejohns requested changes Nov 18, 2024

View reviewed changes

sudha-murthy added 8 commits November 18, 2024 16:32

DAS-2236 - updates based on David's PR feedback

1c8751f

DAS-2236 - updates based on PR feedback

1c29221

DAS-2236 - updated functions and tests based on PR feedback

2f07d1a

DAS-2236 - renamed add_bounds_variables

afe85d8

DAS-2236 - updated unit tests

a3680c5

DAS-2236 - updates to unit tests

57744a6

DAS-2236 - added unit tests for get_range_strings

8590760

DAS-2236 - added unit tests for get_prefetch_variables

3b0a149

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DAS-2236 - Updates to methods that calculate the points used to create the dimensions #18

DAS-2236 - Updates to methods that calculate the points used to create the dimensions #18

sudha-murthy commented Nov 14, 2024

owenlittlejohns left a comment

owenlittlejohns Nov 18, 2024

sudha-murthy Nov 19, 2024

owenlittlejohns Nov 18, 2024

sudha-murthy Nov 19, 2024

owenlittlejohns Nov 18, 2024

sudha-murthy Nov 19, 2024

owenlittlejohns Nov 18, 2024

owenlittlejohns Nov 18, 2024

sudha-murthy Nov 18, 2024

sudha-murthy Nov 20, 2024 •

edited

Loading

owenlittlejohns Nov 18, 2024

sudha-murthy Nov 19, 2024

owenlittlejohns Nov 18, 2024

sudha-murthy Nov 19, 2024

owenlittlejohns Nov 18, 2024

owenlittlejohns Nov 18, 2024

sudha-murthy Nov 20, 2024

owenlittlejohns Nov 18, 2024

sudha-murthy Nov 20, 2024

owenlittlejohns Nov 18, 2024

sudha-murthy Nov 20, 2024

D-Auty commented Nov 22, 2024

		@@ -101,20 +101,23 @@ def get_coordinate_variables(
		CF-Convention coordinates metadata attribute. It returns them in a specific
		order [latitude, longitude]"

DAS-2236 - Updates to methods that calculate the points used to create the dimensions #18

Are you sure you want to change the base?

DAS-2236 - Updates to methods that calculate the points used to create the dimensions #18

Conversation

sudha-murthy commented Nov 14, 2024

Description

Jira Issue ID

Local Test Steps

PR Acceptance Checklist

owenlittlejohns left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sudha-murthy Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

D-Auty commented Nov 22, 2024

sudha-murthy Nov 20, 2024 •

edited

Loading