WIP skip over ValueError in _time_reduce #242

micahkim23 · 2017-11-27T07:22:21Z

Closes #202

micahkim23 · 2017-12-03T02:12:05Z

@spencerahill @spencerkclark Let me know what you guys think about the pr.

spencerkclark

@micahkim23 this looks great so far! It would be great if you could add some tests for this new logic.

I would probably start by creating a variable that was not defined in time in our test object library: https://github.com/spencerahill/aospy/blob/develop/aospy/test/data/objects/examples.py. Then I would write tests to see if the time reductions were skipped when using CalcSuite and that new variable, and if an error was raised if a standalone Calc object was created using that variable that included time reductions in dtype_out_time. The tests would probably be best placed in test_automate.py and test_calc_basic.py.

spencerkclark · 2017-12-03T15:26:41Z

aospy/calc.py

+                    msg = ("Var {0} has no time dimension "
+                           "for the given time reduction "
+                           "{1}".format(var.name, reduction))
+                    logging.info(msg)


Per #202 (comment), let's raise an exception here instead of just logging a message (probably a ValueError is appropriate here).

for testing, are the only possible values for dtype_out_time: None, some subset of ['av', 'std, 'ts', 'reg.av', 'reg.std', 'reg.ts', 'None'], or individual strings of: 'av', 'std, 'ts', 'reg.av', 'reg.std', 'reg.ts', 'None'?

micahkim23 · 2017-12-03T21:21:26Z

aospy/automate.py

+        specs = self._combine_core_aux_specs()
+
+        for spec in specs:
+            spec['dtype_out_time'] = _prune_invalid_time_reductions(spec)


is spec['dtype_out_time'] always a tuple? I'm confused because in here https://github.com/spencerahill/aospy/blob/develop/aospy/calc.py#L153-L156, it's possible for dtype_out_time to be a string, but then it's changed back to a tuple.

I think it's reasonably safe to say here that it will be an iterable of some kind; the documentation states that a list is required in the calc_suite_specs dictionary:
http://aospy.readthedocs.io/en/latest/api.html?highlight=calcsuite#aospy.automate.submit_mult_calcs.

spencerahill

Thanks @micahkim23 . I think we're most of the way there. A few things though that need fixing or I'm not totally understanding.

spencerahill · 2017-12-03T22:32:44Z

aospy/test/data/objects/examples.py

@@ -47,6 +47,13 @@ def total_precipitation(convection_rain, condensation_rain):
    models=[example_model]
 )

+precip_largescale = Var(


Why is this new Var object needed?

I just needed to test a Var object with def_time=False.

OK. Let's rename it to make that explicit; otherwise it's confusing why there are two nearly identical objects. E.g. var_not_time_defined = Var(name='var_no_time_def', def_time=False). You can leave alt_names and description unspecified.

spencerahill · 2017-12-03T22:35:14Z

aospy/test/test_automate.py

+
+
+@pytest.mark.parametrize(
+    ('var'),


Here and elsewhere: parentheses around a single object do nothing: ('var') == 'var'. So I would omit them, except where they are used to break expressions over multiple lines.

Conversely, if there is a trailing comma, it defines a length-1 tuple: e.g. ('var',).

spencerahill · 2017-12-03T22:39:52Z

aospy/test/test_automate.py

+    spec = {
+        'var': var
+    }
+    for i in range(1, 8):


To be more explicit, replace range(1, 8) with range(1, len(time_options) + 1).

You are using the string 'None', instead of the built-in None object. I think we want the latter.

spencerahill · 2017-12-03T22:47:01Z

aospy/test/test_calc_basic.py

+     ('None'),
+     ([])]
+)
+def test_calc_object_no_time_options(dtype_out_time):


This test doesn't assert anything; it will only fail if the CalcInterface call fails. So let's add an assertion about the dtype_out_time in the resulting CalcInterface object.

spencerahill · 2017-12-03T22:51:36Z

aospy/test/test_calc_basic.py

+        CalcInterface(**test_params)
+
+
+@pytest.mark.fixture


The fixture decorator shouldn't be used on a test; instead it's used to prepare the data needed for a test. See https://docs.pytest.org/en/latest/fixture.html#fixtures

spencerahill · 2017-12-03T22:53:13Z

docs/whats-new.rst

@@ -51,6 +51,9 @@ Bug Fixes
  writing to netCDF files to avoid bugs when using ``libnetcdf``
  version 4.5.0 (:pull:`235`).  By `Spencer Hill
  <https://github.com/spencerahill>`_.
+- Skip ``Calc`` object calculation that computes time average of


Expand a bit: note that this refers to when using CalcSuite (and thus submit_mult_calcs), and that Calc now raises a ValueError when instantiated with a non-time-defined variable but with one or more time-defined reductions.

spencerahill · 2017-12-04T16:30:04Z

docs/whats-new.rst

- Skip ``Calc`` object calculation that computes time average of
-  a variable with no time dimension. (closes :issue:`202`
-  via :pull:`242`). By `Micah Kim <https://github.com/micahkim23>`_.
+- In ``CalcSuite`` when calling ``submit_mult_calc``, calculations


CalcSuite doesn't call submit_mult_calc; it's the other way around.

spencerahill · 2017-12-04T16:42:44Z

aospy/test/test_automate.py

@@ -351,3 +354,24 @@ def test_permute_aux_specs(self, calc_suite, calcsuite_specs):
        assert len(actual) == len(expected)
        for act in actual:
            assert act in expected
+
+
+@pytest.mark.parametrize(


Minor style note: splitting across lines like this, with an open parentheses/bracket/curly bracket and then an immediate line break, is usually only needed when either

The items/expression within the overall parentheses/bracket/curly bracket is long enough that this helps it fit onto fewer lines, or at least look nicer.

The items aren't themselves too long to fit on one line each, but there are a lot of them, so it makes it easier to read.

In this case, you don't need to split lines at all; it would fit into a single line:

@pytest.mark.parametrize('var', [precip_largescale, condensation_rain])

Also note more meaningless parentheses; please omit.

spencerahill · 2017-12-04T16:44:53Z

aospy/test/test_calc_basic.py

@@ -160,6 +161,56 @@ def setUp(self):
        }


+test_params = {


nit: indentation is longer than the standard 4 spaces.

(C.f. comment above re: line-splitting: for me this particular case falls into a gray area of intermediate length, where it could work throwing everything onto as few lines as possible, but it's also long enough that what you've done also works.)

spencerahill · 2017-12-04T16:45:02Z

aospy/test/test_calc_basic.py

+}
+
+
+@pytest.mark.parametrize(


More unneeded parentheses + line breaks

spencerahill · 2017-12-04T16:45:17Z

aospy/test/test_calc_basic.py

+        assert calc.dtype_out_time == tuple([dtype_out_time])
+
+
+@pytest.mark.parametrize(


More unneeded parentheses

spencerahill · 2017-12-04T16:47:12Z

docs/whats-new.rst

@@ -51,6 +51,12 @@ Bug Fixes
  writing to netCDF files to avoid bugs when using ``libnetcdf``
  version 4.5.0 (:pull:`235`).  By `Spencer Hill
  <https://github.com/spencerahill>`_.
+- In ``CalcSuite`` when calling ``submit_mult_calc``, calculations


Other way around: submit_mult_calc calls CalcSuite. How about "CalcSuite (and thus submit_mult_calcs) now skips calculations that involve..."

spencerahill · 2017-12-04T16:52:53Z

aospy/test/data/objects/examples.py

@@ -47,6 +47,13 @@ def total_precipitation(convection_rain, condensation_rain):
    models=[example_model]
 )

+precip_largescale = Var(


OK. Let's rename it to make that explicit; otherwise it's confusing why there are two nearly identical objects. E.g. var_not_time_defined = Var(name='var_no_time_def', def_time=False). You can leave alt_names and description unspecified.

spencerkclark

Almost there @micahkim23! Just a few more small things from me.

spencerkclark · 2017-12-04T18:37:25Z

aospy/automate.py

@@ -22,6 +22,7 @@
 _REGIONS_STR = 'regions'
 _VARIABLES_STR = 'variables'
 _TAG_ATTR_MODIFIERS = dict(all='', default='default_')
+_TIME_DEFINED_REDUCTIONS = ['av', 'std', 'ts', 'reg.av', 'reg.std', 'reg.ts']


For now I think it might be a good idea to keep the definition of these only in calc.py and import them here. This way we won't need to remember to maintain this list in two places.

from calc import _TIME_DEFINED_REDUCTIONS

spencerkclark · 2017-12-04T18:38:13Z

aospy/test/test_calc_basic.py

+
+
+@pytest.mark.parametrize(
+    ('dtype_out_time'),


I think you can remove the parentheses here too.

spencerkclark · 2017-12-04T18:38:35Z

aospy/test/test_calc_basic.py

+}
+
+
+@pytest.mark.parametrize(('dtype_out_time'), [None, 'None', []])


I think you can remove the parentheses around 'dtype_out_time' here. Also, is there a reason we are testing with the value 'None'?

spencerkclark · 2017-12-04T18:45:46Z

aospy/test/test_calc_basic.py

+
+
+def test_calc_object_time_options():
+    time_options = ['av', 'std', 'ts', 'reg.av', 'reg.std', 'reg.ts', 'None']


Per @spencerahill's earlier comment, should we use None rather than 'None' here?

aospy/aospy/calc.py

Line 486 in 67ec619

'None': lambda xarr: xarr,

@spencerkclark I thought 'None' was a possible time reduction.

Good catch @micahkim23! @spencerahill was this intentional? Should we change it?

Woops! Yes, good call @micahkim23 . Sorry if any comments I made previously were confused on this point.

Since 'None' appears to just be a synonym for 'ts', I'd say we just remove it entirely. So there's only ['ts', 'av', 'std', 'reg.ts', 'reg.av', 'reg.std'].

Since custom reductions are a big priority moving forward (#208), this will get refactored soon-ish anyways.

spencerahill · 2017-12-05T01:57:58Z

We tempted the AppVeyor gods too long 😜

I'd say this is good to go. Thanks @spencerkclark for the reviews and esp. @micahkim23 for the PR! And sorry again for my leading you astray on the None business. Nice to have this in.

micahkim23 force-pushed the timeoptions branch from 49861d8 to 1a5594c Compare November 29, 2017 07:28

spencerahill mentioned this pull request Nov 29, 2017

Skip dtype_out_time options that cause errors? #202

Closed

micahkim23 force-pushed the timeoptions branch from 1a5594c to cdae831 Compare December 3, 2017 02:06

skip calcs that compute time av of variables with no time dim

f767c0b

micahkim23 force-pushed the timeoptions branch from cdae831 to f767c0b Compare December 3, 2017 02:08

micahkim23 added 2 commits December 2, 2017 18:22

add function description

7a2fc3b

minor visual change

dfe9fbb

spencerkclark reviewed Dec 3, 2017

View reviewed changes

micahkim23 added 2 commits December 3, 2017 11:06

raise ValueError in Calc.__init__

e0b268a

test calc object creation raises ValueError

bb13de1

micahkim23 commented Dec 3, 2017

View reviewed changes

micahkim23 added 3 commits December 3, 2017 13:42

test individual strings

c470b90

test _prune_invalid_time_reductions

92ec066

forgot to test []

786c6b5

spencerahill requested changes Dec 3, 2017

View reviewed changes

micahkim23 added 2 commits December 3, 2017 16:00

code review fixes

9b04316

flake8 error

a1f2fa4

spencerahill requested changes Dec 4, 2017

View reviewed changes

more code review fixes

8d6399b

spencerkclark reviewed Dec 4, 2017

View reviewed changes

micahkim23 added 2 commits December 4, 2017 11:03

more fixes

3c5f66c

remove 'None' time reduction

8024c94

spencerahill approved these changes Dec 5, 2017

View reviewed changes

spencerahill merged commit 841a756 into spencerahill:develop Dec 5, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP skip over ValueError in _time_reduce #242

WIP skip over ValueError in _time_reduce #242

micahkim23 commented Nov 27, 2017

micahkim23 commented Dec 3, 2017

spencerkclark left a comment

spencerkclark Dec 3, 2017

micahkim23 Dec 3, 2017 •

edited

Loading

spencerkclark Dec 3, 2017

micahkim23 Dec 3, 2017 •

edited

Loading

spencerkclark Dec 3, 2017 •

edited

Loading

spencerahill left a comment

spencerahill Dec 3, 2017

micahkim23 Dec 3, 2017

spencerahill Dec 4, 2017

spencerahill Dec 3, 2017

spencerahill Dec 3, 2017

spencerahill Dec 3, 2017

spencerahill Dec 3, 2017

spencerahill Dec 3, 2017

spencerahill Dec 3, 2017

spencerahill Dec 4, 2017

spencerahill Dec 4, 2017

spencerahill Dec 4, 2017

spencerahill Dec 4, 2017

spencerahill Dec 4, 2017

spencerahill Dec 4, 2017

spencerahill Dec 4, 2017

spencerkclark left a comment

spencerkclark Dec 4, 2017

spencerkclark Dec 4, 2017

spencerkclark Dec 4, 2017

spencerkclark Dec 4, 2017

micahkim23 Dec 4, 2017 •

edited

Loading

spencerkclark Dec 4, 2017

spencerahill Dec 4, 2017

spencerahill commented Dec 5, 2017 •

edited

Loading

		assert calc.dtype_out_time == tuple([dtype_out_time])


		@pytest.mark.parametrize(

		}


		@pytest.mark.parametrize(('dtype_out_time'), [None, 'None', []])



		def test_calc_object_time_options():
		time_options = ['av', 'std', 'ts', 'reg.av', 'reg.std', 'reg.ts', 'None']

		CalcInterface(**test_params)


		@pytest.mark.fixture

WIP skip over ValueError in _time_reduce #242

WIP skip over ValueError in _time_reduce #242

Conversation

micahkim23 commented Nov 27, 2017

micahkim23 commented Dec 3, 2017

spencerkclark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

micahkim23 Dec 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

micahkim23 Dec 3, 2017 • edited Loading

Choose a reason for hiding this comment

spencerkclark Dec 3, 2017 • edited Loading

Choose a reason for hiding this comment

spencerahill 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

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

spencerkclark 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

micahkim23 Dec 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spencerahill commented Dec 5, 2017 • edited Loading

micahkim23 Dec 3, 2017 •

edited

Loading

micahkim23 Dec 3, 2017 •

edited

Loading

spencerkclark Dec 3, 2017 •

edited

Loading

micahkim23 Dec 4, 2017 •

edited

Loading

spencerahill commented Dec 5, 2017 •

edited

Loading