This repository has been archived by the owner on Sep 30, 2024. It is now read-only.
Wind observation coordinates #62
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We'll need the location of the observations if we're going to plot them on a map, so I added a named tuple to capture the coordinates and serialize to an array of [longitude, latitude]. Serializing to an object with "longitude" and "latitude" for each observation starts to really increase the size of the data when it's transmitted.
Updated all the tests to expect coordinates for the VectorVariables. I am wondering if the coordinates should live with the VectorDiag, though, because they're duplicated across both the observed and forecast values, so we're doubling the amount of data we report for coordinates.
Changed the expected results of the test_wind_diag test to reduce duplicate coordinates. I actually suspect this could be one coordinate array for the entire response, because the first and third minimization loops should be working from the same set of observations. I marked a number of the tests in test_diag.py as expected failures for now just to clean up the output.
Back to all green tests after removing coordinates from the VectorVariable and moving them up to the VectorDiag.
Refactor the list comprehension from wind() into a utility function that takes an array of longitudes and an array of latitudes and returns a list of Coordinate tuples. This ensures that wind() can just orchestrate calls to collaborators and includes no "business logic" for cleaner testing. It's going to mean a more complicated test with mocks for wind(), but it also makes it easier to isolate tests for the coordinates.
Use the new coordinate_pairs_from_vectors in the wind() function to generate the list off observation coordinates for the variables. Rewrote test_wind() to ensure that we create the test data before we mock the various collaborators. What we were doing originally was interfering with call counts to the mocked function. Mocking VectorDiag and then using diag.VectorDiag to create the expected result counted as a call to the mocked class (of course), so when we try to test the mocked VectorDiag and assert that it's only called once by the wind() function, the test fails, because the mock was called twice: once in the test, once in the wind() function. It's possible we don't even need to mock the return values here. We could probably just assert that the return value of one function (whatever that is) gets passed as an argument to the next collaborator.
I was right, we can just test that the calls to one mocked function are
made with the return value of another mocked function, there's no need
for us to set the return value for most of these mocked functions.
I confirmed that
mock_VectorVariable.return_value != mock_VectorVariable.from_vectors.return_value
so we can be sure that we're testing the behavior of the function with
fine enough detail to catch changes or mistakes like not using the
from_vectors factory.
There's really no need to store the VectorDiag in a data variable to return it. The code is short enough now that directly returning the new VectorDiag object is clear.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Include coordinates for the observations in the diagnostic data returned by the wind API so that we can plot the wind vectors on a map.