Grib1unit10 additional #184

pp-mo · 2012-10-31T15:32:55Z

Added additional GRIB1 and GRIB2 time units support.
Also enhanced test_grib_load, with (beginnings of) improved Mock-style testing methods, as suggested by @rhattersley in ##171 (and private discussions)

pp-mo · 2012-10-31T15:38:10Z

There is still plenty of work to do making the grib tests quicker and more focussed. Hopefully the 'Mock' approach can be extended to assist this.
See also #110

bblay · 2012-11-05T11:30:30Z

lib/iris/tests/test_grib_load.py

+        Implement only a few methods, just enough to allow GribWrapper creation.
+    """
+
+    class GribInternalError(Exception):


I assume we can't just use the actual class from the grib api instead?

We could, I hadn't thought of that.
However, it will introduce an extra "knowledge dependency", as we need to (know how to) make one so we can raise it in the faking code (below).
No actual behaviour seems to be used, though, so it is terribly easy to 'fake'
-- and then that is also a statement of precisely how much interface we think is required (!), which I quite like.

Good point regarding dependency.
Seems desirable to be completely independent of the real grib api.

bblay · 2012-11-05T13:02:53Z

Good job. Made some comments.

pp-mo · 2012-11-05T16:33:40Z

lib/iris/tests/test_grib_load.py

+
+
+@contextmanager
+def Fakeup_Gribapi_Context(fake_gribapi_class=Fake_GribApi):


I'm still having doubts about the worth of this, in place of a simple "try.. finally" block.
@rhattersley put me up to this, but having done it, I'm worried it adds very little in this case, and is considerably less transparent.

It doesn't even simplify the usage code that much : Compare

with Fakeup_Gribapi_Context(): <stuff>...

with

try: orig_gribapi_module = iris.fileformats.grib.gribapi iris.fileformats.grib.gribapi = Fake_GribApi() <stuff>... finally: iris.fileformats.grib.gribapi = orig_gribapi_module

I really like it. It's safe and neat.
In fact, I wrote a praising comment but didn't post it.
I'd like to see this become the standard for module mocking in Iris.

Continuing the great tradition of turning nouns into verbs... could we PEP8 the names please. I'm looking at you "Fake_GribApi", and you "Fakeup_Gribapi_Context"!

(While you're at it, the use of the word "context" doesn't really add much and is a bit reverse-Polish.)

Correction - please could you convert to using http://pypi.python.org/pypi/mock

An example of mocking can be found here: #229

pp-mo · 2012-11-05T16:51:11Z

Re: your various "remove / unnecessary" comments.
If the code was 'just mine' I'd keep these lines as they will be useful again when adding more testcases.
There are choices

remove altogether -- wastes time reimplementing debug output when you rework this code
leave as-is
- breaks style rules
- highlights extra 'code' that may be handy, but never runs and may not actually work
include as viable code controlled by a 'debugging' flag
- harder to understand what it is
- main function is harder to read, as extra code breaks it up (as no longer shown 'greyed out')
- looks like 'real' code but actually never runs, so will go stale

pp-mo · 2012-11-05T16:53:50Z

@bblay : thanks for comments.
Where do we take it from here?

bblay · 2012-11-05T20:37:21Z

I think the only thing to do is:

The "remove" stuff will have to go before it is acceptable to the group.

I think the grib1 key in the grib2 dict is harmless.

The rest is fine as it is:

The refactor doesn't have to be done.
The context manager is good, please keep it.
I agree we should not use the real gribapi exception.

pp-mo · 2012-12-03T13:19:32Z

Changes to cover the issues previously raised.
@rhattersley I'm not entirely sure what was intended from use of 'mock' -- does this address it?

rhattersley · 2012-12-03T16:34:30Z

lib/iris/tests/test_grib_load.py

@@ -30,10 +34,49 @@
 import iris.util
 import iris.tests.stock

+import mock
+
+import gribapi


This should be in the 3rd-party section alongside matplotlib.pyplot. Similarly for mock and numpy.

The datetime and contextlib imports should be moved to sit with os and warnings.

rhattersley · 2012-12-03T17:03:34Z

I'm not entirely sure what was intended from use of 'mock' -- does this address it?

The mock module is just there as a tool to help with writing focussed tests. In this case the GribWrapper class is not really structured to make unit tests easy, so even with mock it gets pretty messy. In particular, there's lots of work done in the constructor so it's hard to test a method without faking lots of things.

With hindsight, I suspect we would have been better served by first making a PR which removed the code in the GribWrapper constructor (hard). Then making a second PR to add the new unit handling (now very easy).

rhattersley · 2012-12-03T17:42:16Z

lib/iris/fileformats/grib.py

+        return self._time_unit_stringandsecs()[0]
+
+    def _time_unit_as_seconds(self):
+        return self._time_unit_stringandsecs()[1]


The inter-relationships between _time_unit_as_string, _time_unit_as_seconds, and _time_unit_stringandsecs are overly complex. Wouldn't it'd be simpler and more transparent just to have:

_time_unit_as_string: the code from the first half of _time_unit_stringandsecs

_time_unit_as_seconds: calls _time_unit_as_string to get the string, then decodes it using the code from the second half of _time_unit_stringandsecs

Having done that, it seems pretty odd to convert the GRIB values into a string just to decode the string into numbers again. How about something vaguely (please don't take it too literally) like:

class GribWrapper: edition_1 = {13: ('15 minutes', 15 * 60), 14: ('30 minutes', 30 * 60)} edition_2 = {13: ...} edition_any = {0: ('minutes', 60), ...} def _time_unit(self): if edition == 1: answer = self.edition_1.get(timeunit_num) or self.edition_any(timeunit_num) return answer def _time_unit_as_string(self): return self._time_unit()[0] def _time_unit_as_seconds(self): return self._time_unit()[1]

Oddly, the three-part solution is back again!

rhattersley · 2012-12-03T17:44:58Z

There are lots of PEP8 issues which could do with ironing out. You may well find the pep8 utility handy.

bblay · 2012-12-04T11:34:46Z

With hindsight, I suspect we would have been better served by first making a PR which removed the code in the GribWrapper constructor (hard)

#182 would probably do away with this. Much of the grib wrapper code is just there to support the rules system (the "extra keys").

pp-mo · 2012-12-04T14:16:01Z

Hopefully now have covered the following points...

imports : "This should be in the 3rd-party section ..."
docstrings : "This doesn't match the local docstring style. Nor do the other docstrings in this PR."
pep8 : "There are lots of PEP8 issues which could do with ironing out..."

Outstanding work stil to come:

grib.py timefunctions : "inter-relationships between _time_unit_as_string, _time_unit_as_seconds ..."
test_grib fail-tests : "It'd be good to have a test or two that fails too."

rhattersley · 2012-12-04T14:38:16Z

lib/iris/fileformats/grib.py

+    def _time_unit_stringandsecs(self):
+        """
+        Interpret message time unit as udunits-string, and float(seconds).
+        """


For a multi-line docstring there should be a blank line before the closing quotes.

pp-mo · 2012-12-05T12:04:10Z

Rather extensively refactored : Probably overkill, but I hope it addresses the outstanding issues.

Anyway, I certainly learnt a few things.
NOTE: I'm not terribly happy with the way fake-message construction + operations are packaged.
These would be nicer as a class, but the trivial way needs to subclas 'dict', which you can't -- roll on, Python3 ??

Please review.

rhattersley · 2012-12-05T12:17:48Z

These would be nicer as a class, but the trivial way needs to subclas 'dict', which you can't -- roll on, Python3 ??

Qué?

Python 2.7.2 (default, Oct  1 2012, 15:56:20) 
[GCC 4.4.6 20110731 (Red Hat 4.4.6-3)] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> class Foo(dict):
...  def __getitem__(self, key):
...   return 'wibble'
... 
>>> Foo()['thing']
'wibble'

rhattersley · 2012-12-05T13:07:36Z

lib/iris/fileformats/grib.py

+        u_secs = int(u_num) * u_basesecs
+        if not (u_secs > 0.0):
+            raise Exception(
+                'Unexpected time unit string : "{0}"'.format(units_string)


It doesn't matter in this instance, but in general for quoting a string you're better off using {!r} to get the repr version - it automatically handles the different kinds of quotes and any necessary escaping (plus it's shorter and less "noisy"). i.e. 'Unexpected time unit string : {!r}'.format(units_string)

pp-mo · 2012-12-05T14:28:58Z

These would be nicer as a class ... roll on, Python3 ??

Qué?

Sorry, it seems I was just out-of-date on this.
Probably not worth changing it now ?

Ok, I've just done it now
-- see pp-mo@3f6ed5a

pp-mo · 2012-12-05T14:31:27Z

Pull Request #1: Alternate string/seconds scheme

Thanks @rhattersley. If you think so...
Is it perhaps better to enclose all the information in the function?
-- asin pp-mo@0e667da
?

rhattersley · 2012-12-06T11:15:04Z

Ok, I've just done it now
-- see pp-mo/iris@3f6ed5a

You've done it already - so if you want to include it in the PR then feel free.

rhattersley · 2012-12-06T11:18:35Z

Is it perhaps better to enclose all the information in the function?
-- asin pp-mo/iris@0e667da
?

Personally, I think they're better as a module constant, rather than a local variable. They are constant after all. And it avoids Python having to make a fresh dict each time. (Yes - I know there's no such thing as a true constant in Python!)

pp-mo · 2012-12-07T12:01:18Z

Applied patches : fake-message-objects and alternate-string/seconds
Rebased (hence the mess in the commits box?)

rhattersley · 2012-12-10T16:39:56Z

Thanks for the re-base - looks good now. I think it would benefit from a squash though, if you're happy to give that a go...?

pp-mo · 2012-12-10T16:49:05Z

I can compress the log to just one entry "Support extra GRIB timeunit codes".
This rather glosses over the test enhancement work.
@rhattersley does that seem appropriate, or do we need more detail in the log?

rhattersley · 2012-12-10T16:53:39Z

I wouldn't hurt to have an extra line or two in the commit message. But I'm not that bothered either way. You choose 😉

- also small tweak to GribWrapper __init__, to prevent possible infinite loops

Additional GRIB time units

rhattersley · 2012-12-10T17:24:15Z

Phew! Thanks for hanging in there @pp-mo! 😄

bblay reviewed Nov 5, 2012
View reviewed changes

pp-mo reviewed Nov 5, 2012
View reviewed changes

bblay mentioned this pull request Nov 6, 2012

GRIB1 duplicate rule #180

Merged

pp-mo mentioned this pull request Nov 14, 2012

Grib steps bug #200

Merged

rhattersley reviewed Dec 3, 2012
View reviewed changes

rhattersley reviewed Dec 4, 2012
View reviewed changes

rhattersley reviewed Dec 5, 2012
View reviewed changes

rhattersley mentioned this pull request Dec 5, 2012

Alternate string/seconds scheme pp-mo/iris#1

Closed

ghost assigned rhattersley Dec 6, 2012

pp-mo added 2 commits December 10, 2012 17:05

replace test_grib_load.py testcase "test_fp_units" with a Mock test.

14abf29

- also small tweak to GribWrapper __init__, to prevent possible infinite loops

enable all non-calendar grib time-unit codes.

f7bac53

rhattersley added a commit that referenced this pull request Dec 10, 2012

Merge pull request #184 from pp-mo/grib1unit10_additional

e764fbf

Additional GRIB time units

rhattersley merged commit e764fbf into SciTools:master Dec 10, 2012

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Grib1unit10 additional #184

Grib1unit10 additional #184

pp-mo commented Oct 31, 2012

pp-mo commented Oct 31, 2012

bblay Nov 5, 2012

pp-mo Nov 5, 2012

bblay Nov 5, 2012

bblay commented Nov 5, 2012

pp-mo Nov 5, 2012

bblay Nov 5, 2012

rhattersley Nov 6, 2012

rhattersley Nov 6, 2012

bblay Nov 27, 2012

pp-mo commented Nov 5, 2012

pp-mo commented Nov 5, 2012

bblay commented Nov 5, 2012

pp-mo commented Dec 3, 2012

rhattersley Dec 3, 2012

rhattersley commented Dec 3, 2012

rhattersley Dec 3, 2012

rhattersley commented Dec 3, 2012

bblay commented Dec 4, 2012

pp-mo commented Dec 4, 2012

rhattersley Dec 4, 2012

pp-mo commented Dec 5, 2012

rhattersley commented Dec 5, 2012

rhattersley Dec 5, 2012

pp-mo commented Dec 5, 2012

pp-mo commented Dec 5, 2012

rhattersley commented Dec 6, 2012

rhattersley commented Dec 6, 2012

pp-mo commented Dec 7, 2012

rhattersley commented Dec 10, 2012

pp-mo commented Dec 10, 2012

rhattersley commented Dec 10, 2012

rhattersley commented Dec 10, 2012



		@contextmanager
		def Fakeup_Gribapi_Context(fake_gribapi_class=Fake_GribApi):

Grib1unit10 additional #184

Grib1unit10 additional #184

Conversation

pp-mo commented Oct 31, 2012

pp-mo commented Oct 31, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bblay commented Nov 5, 2012

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

pp-mo commented Nov 5, 2012

pp-mo commented Nov 5, 2012

bblay commented Nov 5, 2012

pp-mo commented Dec 3, 2012

Choose a reason for hiding this comment

rhattersley commented Dec 3, 2012

Choose a reason for hiding this comment

rhattersley commented Dec 3, 2012

bblay commented Dec 4, 2012

pp-mo commented Dec 4, 2012

Choose a reason for hiding this comment

pp-mo commented Dec 5, 2012

rhattersley commented Dec 5, 2012

Choose a reason for hiding this comment

pp-mo commented Dec 5, 2012

pp-mo commented Dec 5, 2012

rhattersley commented Dec 6, 2012

rhattersley commented Dec 6, 2012

pp-mo commented Dec 7, 2012

rhattersley commented Dec 10, 2012

pp-mo commented Dec 10, 2012

rhattersley commented Dec 10, 2012

rhattersley commented Dec 10, 2012