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

Add RMS iris analysis operator to statistics preprocessor functions #747

Merged

Conversation

pepcos
Copy link
Contributor

@pepcos pepcos commented Aug 12, 2020

Adding RMS iris analysis operator (which allow Area Weighting) to the preprocessor. This allows computing root mean squares in the _area.py and _time.py preprocessors.

Tasks

  • Create an issue to discuss what you are going to do, if you haven't done so already (and add the link at the bottom)
  • This pull request has a descriptive title that can be used in a changelog
  • Add unit tests
  • If writing a new/modified preprocessor function, please update the documentation
  • Circle/CI tests pass. Status can be seen below your pull request. If the tests are failing, click the link to find out why.
  • Codacy code quality checks pass. Status can be seen below your pull request. If there is an error, click the link to find out why. If you suspect Codacy may be wrong, please ask by commenting.

closes #739

@pepcos pepcos marked this pull request as ready for review August 12, 2020 10:14
@bouweandela
Copy link
Member

Thanks for the pull request! Could you please have a look at the failing test on CircleCI? Just click the commit link below, it looks like there is something wrong with the formatting of one of the docstrings. You can build the documentation on your own computer by following these instructions: https://docs.esmvaltool.org/projects/esmvalcore/en/latest/contributing.html#how-to-build-the-documentation-locally

@schlunma schlunma requested review from valeriupredoi and removed request for schlunma August 13, 2020 06:57
@bouweandela
Copy link
Member

Would you also be able to add some unit tests, e.g. similar to:

def test_time_median(self):
"""Test for time meadian of a 1D field."""
data = np.arange((3))
times = np.array([15., 45., 75.])
bounds = np.array([[0., 30.], [30., 60.], [60., 90.]])
cube = self._create_cube(data, times, bounds)
result = climate_statistics(cube, operator='median')
expected = np.array([1.])
assert_array_equal(result.data, expected)

and
def test_area_statistics_median(self):
"""Test for area average of a 2D field."""
result = area_statistics(self.grid, 'median')
expected = np.array([1.])
self.assert_array_equal(result.data, expected)

?

@pepcos
Copy link
Contributor Author

pepcos commented Aug 13, 2020

Hi @bouweandela

I've never done unit testing so correct me if I'm wrong: Should I add a couple of functions to test_area.py and test_time.py in order to check that rms works fine?

Thanks!

@bouweandela
Copy link
Member

bouweandela commented Aug 13, 2020

Yes, that's the idea. You can just copy/paste the code of the examples mentioned above, replace 'median' by 'rms' and make sure the expected numbers match the computed result. To run the tests in a particular file, you can run pytest tests/unit/preprocessor/_time/test_time.py, see also https://docs.pytest.org

Copy link
Member

@bouweandela bouweandela left a comment

Choose a reason for hiding this comment

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

Thanks! Could you please carefully check that you update the documentation for all preprocessor functions? It looks like you missed a few: 'rms' is missing at least in timeseries_filter, and in preprocessor.rst: zonal_statistics and meridional_statistics.

@pepcos
Copy link
Contributor Author

pepcos commented Aug 14, 2020

Thanks for checking @bouweandela !

@sloosvel coould you have a look at it? Thanks.

@sloosvel
Copy link
Contributor

Looks fine to me, good job @pcosbsc .

Copy link
Member

@bouweandela bouweandela left a comment

Choose a reason for hiding this comment

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

@mattiarighi or @valeriupredoi Could you please test?

@bouweandela
Copy link
Member

@sloosvel or @jvegasbsc Would you have time to run a few recipes using the new functionality for testing, so we can still include this in v2.1?

@sloosvel
Copy link
Contributor

sloosvel commented Oct 1, 2020

Works fine in both the area and time preprocessors.

@bouweandela bouweandela merged commit c411572 into ESMValGroup:master Oct 1, 2020
@bouweandela bouweandela changed the title added RMS iris analysis operator Add RMS iris analysis operator to statistics preprocessor functions Oct 1, 2020
@bouweandela bouweandela added the preprocessor Related to the preprocessor label Oct 1, 2020
@pepcos pepcos deleted the add_rms_iris_analysis_operator branch October 1, 2020 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preprocessor Related to the preprocessor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adding area weighted RMS to area_statistics
3 participants