Issue/233 data validation #258

peterdudfield · 2021-10-21T21:43:12Z

Pull Request

Description

Add validation for

sun
satellite
nwp
pv
gsp

Fixes #233
#29

How Has This Been Tested?

Add some unittests to check the errors

No
Yes

Checklist:

My code follows OCF's coding style guidelines
I have performed a self-review of my own code
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works
I have checked my code and corrected any misspellings

jacobbieker

Looks good! Just a few comments on maybe simplifying some of the asserts, but thanks for adding all the validation!

jacobbieker · 2021-10-22T07:45:51Z

nowcasting_dataset/data_sources/nwp/nwp_model.py

@@ -24,5 +24,5 @@ class NWP(DataSourceOutput):
    @classmethod
    def model_validation(cls, v):
        """ Check that all values are not NaNs """
-        assert (v.data != np.nan).all(), "Some nwp data values are NaNs"
+        assert (~np.isnan(v.data)).all(), "Some nwp data values are NaNs"


What about checking for positive and negative infinity? with

Suggested change

assert (~np.isnan(v.data)).all(), "Some nwp data values are NaNs"

assert (~np.isfinite(v.data)).all(), "Some nwp data values are NaNs or infinite"

Yeah, I agree about using np.isfinite to test for NaNs, +inf and -inf. But, if we use np.isfinite, then we'll also need to remove the ~ and tweak the string 🙂

Suggested change

assert (~np.isnan(v.data)).all(), "Some nwp data values are NaNs"

assert (np.isfinite(v.data)).all(), "Some nwp data values are NaNs or infinities"

I'm gona go with both isnan and isinf. I this leads to slightly more verbose error messages

For example
assert (~isnan(v.data)).all(), f"Some pv data values are NaNs"
assert (~isinf(v.data)).all(), f"Some pv data values are Infinite"

jacobbieker · 2021-10-22T07:47:00Z

nowcasting_dataset/data_sources/satellite/satellite_model.py

@@ -27,5 +27,6 @@ class Satellite(DataSourceOutput):
    @classmethod
    def model_validation(cls, v):
        """ Check that all values are non negative """
-        assert (v.data != np.NaN).all(), f"Some satellite data values are NaNs"
+        assert (~np.isnan(v.data)).all(), f"Some satellite data values are NaNs"


Same as for NWP

Suggested change

assert (~np.isnan(v.data)).all(), f"Some satellite data values are NaNs"

assert (~np.isfinite(v.data)).all(), f"Some satellite data values are NaNs or infinite"

Same as above 🙂

Suggested change

assert (~np.isnan(v.data)).all(), f"Some satellite data values are NaNs"

assert (np.isfinite(v.data)).all(), "Some nwp data values are NaNs"

jacobbieker · 2021-10-22T07:48:39Z

nowcasting_dataset/data_sources/topographic/topographic_model.py

@@ -21,5 +21,6 @@ class Topographic(DataSourceOutput):
    @classmethod
    def model_validation(cls, v):
        """ Check that all values are non NaNs """
-        assert (v.data != np.NaN).all(), f"Some topological data values are NaNs"
+        assert (~np.isnan(v.data)).all(), f"Some topological data values are NaNs"
+        assert (v.data != np.Inf).all(), f"Some topological data values are Infinite"


Possibly just combining the two asserts might be a bit simpler?

Suggested change

assert (v.data != np.Inf).all(), f"Some topological data values are Infinite"

assert (np.isfinite(v.data)).all(), f"Some topological data values are Infinite or NaNs"

See comment above, think I will keep them separate

jacobbieker · 2021-10-22T07:49:25Z

nowcasting_dataset/data_sources/gsp/gsp_model.py

+        assert (~np.isnan(v.data)).all(), f"Some gsp data values are NaNs"
+        assert (v.data != np.Inf).all(), f"Some gsp data values are Infinite"


Possibly combine the two asserts with

Suggested change

assert (~np.isnan(v.data)).all(), f"Some gsp data values are NaNs"

assert (v.data != np.Inf).all(), f"Some gsp data values are Infinite"

assert (np.isfinite(v.data)).all(), f"Some gsp data values are NaNs or infinite"

See comment above, think I will keep them separate

jacobbieker · 2021-10-22T07:50:08Z

nowcasting_dataset/data_sources/pv/pv_model.py

+        assert (~np.isnan(v.data)).all(), f"Some pv data values are NaNs"
+        assert (v.data != np.Inf).all(), f"Some pv data values are Infinite"


Again, possibly combining the two might be a bit cleaner

Suggested change

assert (~np.isnan(v.data)).all(), f"Some pv data values are NaNs"

assert (v.data != np.Inf).all(), f"Some pv data values are Infinite"

assert (np.isfinite(v.data)).all(), f"Some pv data values are NaNs or infinite"

See comment above, think I will keep them separate

jacobbieker · 2021-10-22T07:52:22Z

nowcasting_dataset/data_sources/sun/sun_model.py

+        assert (~np.isnan(v.elevation)).all(), f"Some elevation data values are NaNs"
+        assert (v.elevation != np.Inf).all(), f"Some elevation data values are Infinite"
+


Possibly combine these two, or if we do want to keep them separate, changing it to

Suggested change

assert (~np.isnan(v.elevation)).all(), f"Some elevation data values are NaNs"

assert (v.elevation != np.Inf).all(), f"Some elevation data values are Infinite"

assert (~np.isnan(v.elevation)).all(), f"Some elevation data values are NaNs"

assert (~np.isinf(v.elevation)).all(), f"Some elevation data values are Infinite"

as I believe np.Inf only counts as positive infinity, and wouldn't check for negative infinity

See comment above, think I will keep them separate

Cool, good idea!

jacobbieker · 2021-10-22T07:52:31Z

nowcasting_dataset/data_sources/sun/sun_model.py

+        assert (~np.isnan(v.azimuth)).all(), f"Some azimuth data values are NaNs"
+        assert (v.azimuth != np.Inf).all(), f"Some azimuth data values are Infinite"


Same as above

peterdudfield · 2021-10-22T08:03:04Z

Looks good! Just a few comments on maybe simplifying some of the asserts, but thanks for adding all the validation!

Ill put np.isfinite

Yea good suggestions, I wanted to put but nan check and infinite check with a different error message, so it's easier to debug. I know that its twice as much work, but its seems like it might be worth it

jacobbieker · 2021-10-22T08:04:18Z

Looks good! Just a few comments on maybe simplifying some of the asserts, but thanks for adding all the validation!

Ill put np.isfinite

Yea good suggestions, I wanted to put but nan check and infinite check with a different error message, so it's easier to debug. I know that its twice as much work, but its seems like it might be worth it

Okay, yeah, makes sense! I would then suggest using np.isinf instead of np.isfinite checks both NaN and infinities, while np.isinf checks only infinities

JackKelly

LGTM! Thanks!

peterdudfield added 6 commits October 20, 2021 19:23

add pv, gsp and sun validation

54ae844

Use ~isnan to validator

52ea668

sun elevation between -90 to 90

20311b9

add test for satellite validation

b963934

add gsp and sun validation tests

db36595

sat validation -1 #29

931da61

peterdudfield linked an issue Oct 21, 2021 that may be closed by this pull request

Check for -1 values in sat data #29

Closed

make gsp model check simplier

40e9960

peterdudfield marked this pull request as ready for review October 22, 2021 07:26

peterdudfield requested review from jacobbieker, flowirtz and JackKelly and removed request for jacobbieker October 22, 2021 07:26

jacobbieker approved these changes Oct 22, 2021

View reviewed changes

peterdudfield added 2 commits October 22, 2021 09:08

PR comments - thanks Jacob

581c380

use from xarray.ufuncs import isnan, isinf

9c9ad04

JackKelly approved these changes Oct 22, 2021

View reviewed changes

peterdudfield merged commit d64f242 into main Oct 22, 2021

peterdudfield deleted the issue/233-data-validation branch October 22, 2021 08:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue/233 data validation #258

Issue/233 data validation #258

peterdudfield commented Oct 21, 2021 •

edited

Loading

jacobbieker left a comment

jacobbieker Oct 22, 2021

JackKelly Oct 22, 2021 •

edited

Loading

peterdudfield Oct 22, 2021

jacobbieker Oct 22, 2021

JackKelly Oct 22, 2021

jacobbieker Oct 22, 2021

peterdudfield Oct 22, 2021

jacobbieker Oct 22, 2021

peterdudfield Oct 22, 2021

jacobbieker Oct 22, 2021

peterdudfield Oct 22, 2021

jacobbieker Oct 22, 2021

peterdudfield Oct 22, 2021

JackKelly Oct 22, 2021

jacobbieker Oct 22, 2021

peterdudfield commented Oct 22, 2021

jacobbieker commented Oct 22, 2021 •

edited

Loading

JackKelly left a comment

	assert (~np.isnan(v.data)).all(), "Some nwp data values are NaNs"
	assert (~np.isfinite(v.data)).all(), "Some nwp data values are NaNs or infinite"

	assert (~np.isnan(v.data)).all(), "Some nwp data values are NaNs"
	assert (np.isfinite(v.data)).all(), "Some nwp data values are NaNs or infinities"

	assert (~np.isnan(v.data)).all(), f"Some satellite data values are NaNs"
	assert (~np.isfinite(v.data)).all(), f"Some satellite data values are NaNs or infinite"

	assert (v.data != np.Inf).all(), f"Some topological data values are Infinite"
	assert (np.isfinite(v.data)).all(), f"Some topological data values are Infinite or NaNs"

		assert (~np.isnan(v.elevation)).all(), f"Some elevation data values are NaNs"
		assert (v.elevation != np.Inf).all(), f"Some elevation data values are Infinite"

		assert (~np.isnan(v.azimuth)).all(), f"Some azimuth data values are NaNs"
		assert (v.azimuth != np.Inf).all(), f"Some azimuth data values are Infinite"

Issue/233 data validation #258

Issue/233 data validation #258

Conversation

peterdudfield commented Oct 21, 2021 • edited Loading

Pull Request

Description

How Has This Been Tested?

Checklist:

jacobbieker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JackKelly Oct 22, 2021 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterdudfield commented Oct 22, 2021

jacobbieker commented Oct 22, 2021 • edited Loading

JackKelly left a comment

Choose a reason for hiding this comment

peterdudfield commented Oct 21, 2021 •

edited

Loading

JackKelly Oct 22, 2021 •

edited

Loading

jacobbieker commented Oct 22, 2021 •

edited

Loading