Portable comparisons (for sorting) #1709

QuLogic · 2015-06-24T21:58:57Z

There are a few mixed comparisons (None with int, str with int, etc.) that do not work directly with Python 3. So add an extra element to the sorting key so that those comparisons do not occur (and work similar to Python 2).

QuLogic · 2015-06-24T21:59:55Z

lib/iris/coords.py

Does it even make sense to use the units or coordinate system in the comparison? I'm not even sure they're comparable. Should I leave them out?

Also, thinking about it a bit, those first elements should ensure never comparing None with str, so maybe I can leave out those or ''?

Does it even make sense to use the units or coordinate system in the comparison?

Yes - so we can have consistent (albeit arbitrary) ordering for coordinates which are not equal.

maybe I can leave out those or ''?

👍

QuLogic · 2015-06-24T22:12:02Z

Oh, I forgot I added some usage of six, which means this PR will conflict with #1700. Please merge it soon.

rhattersley · 2015-06-25T08:47:00Z

lib/iris/_merge.py

Could this if be replaced with return (axis_index, isinstance(name, int), str(name))?

Yes, I think so; probably can leave out the str too for the same reason as above.

QuLogic · 2015-06-26T08:55:58Z

I added one more sorting fix, 184e165.

QuLogic · 2015-06-26T09:06:53Z

lib/iris/io/format_picker.py

Hmm, now that I think about it, this one may only work if names are unique? (because __eq__ still uses hash.)

QuLogic · 2015-07-01T06:51:35Z

Maybe that will work better.

QuLogic · 2015-07-07T04:47:54Z

So I've found two more comparison issues:

======================================================================
ERROR: test_multiple_unordered_lbprocs (iris.tests.unit.fileformats.pp_rules.test__all_other_rules.TestCellMethods)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/elliott/code/iris/testdir/conda/envs/env34/lib/python3.4/site-packages/Iris-1.9.0.dev0-py3.4.egg/iris/tests/unit/fileformats/pp_rules/test__all_other_rules.py", line 125, in test_multiple_unordered_lbprocs
    res = _all_other_rules(field)[CELL_METHODS_INDEX]
  File "/home/elliott/code/iris/testdir/conda/envs/env34/lib/python3.4/site-packages/Iris-1.9.0.dev0-py3.4.egg/iris/fileformats/pp_rules.py", line 881, in _all_other_rules
    coord_system=f.coord_system()),
  File "/home/elliott/code/iris/testdir/conda/envs/env34/lib/python3.4/site-packages/Iris-1.9.0.dev0-py3.4.egg/iris/coords.py", line 1346, in from_regular
    if not is_regular(coord) and count > 1:
TypeError: unorderable types: MagicMock() > int()

======================================================================
ERROR: test_multiple_unordered_rotated_lbprocs (iris.tests.unit.fileformats.pp_rules.test__all_other_rules.TestCellMethods)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/elliott/code/iris/testdir/conda/envs/env34/lib/python3.4/site-packages/Iris-1.9.0.dev0-py3.4.egg/iris/tests/unit/fileformats/pp_rules/test__all_other_rules.py", line 136, in test_multiple_unordered_rotated_lbprocs
    res = _all_other_rules(field)[CELL_METHODS_INDEX]
  File "/home/elliott/code/iris/testdir/conda/envs/env34/lib/python3.4/site-packages/Iris-1.9.0.dev0-py3.4.egg/iris/fileformats/pp_rules.py", line 881, in _all_other_rules
    coord_system=f.coord_system()),
  File "/home/elliott/code/iris/testdir/conda/envs/env34/lib/python3.4/site-packages/Iris-1.9.0.dev0-py3.4.egg/iris/coords.py", line 1346, in from_regular
    if not is_regular(coord) and count > 1:
TypeError: unorderable types: MagicMock() > int()

I'm not sure what the expected value should be for these. In those two tests, some fields of the pp mocked data are unset. @marqh Can you shed some light on these since you wrote the tests?

marqh · 2015-07-14T13:34:46Z

the tests that are failing are failing in the iris.coords

the error is due to the fact that a mock object is being evaluated compared to an integer. this is because the tests provide a mocked version of the x_coord+name and _y_coord_name in the mock field

most mock tests only use the parts of the PP field they require

@QuLogic please may you be more specific when you say:

I'm not sure what the expected value should be for these.

which expected values? for which 'these'?

thank you
mark

QuLogic · 2015-07-14T18:41:58Z

The PP loader calls DimCoord.from_regular(f.bzx, f.bdx, f.lbnpt, ..., but none of these fields are set in the mocked object. Those are the fields I mean.

marqh · 2015-07-15T07:39:22Z

The PP loader calls DimCoord.from_regular(f.bzx, f.bdx, f.lbnpt, ..., but none of these fields are set in the mocked object. Those are the fields I mean.

I see. Those metadata elements are not required for this test, the only important facet for setting the cell_methods for the lbproc codes is whether the grid is rotated or not, so I mocked out the behaviour with the _{x|y}_coord_name

I'm afraid I don't understand the nature of the test failure. I can see that the coord_system is being run on the field, but I don't see why this is now failing

we could make up a set of arbitrary values for the field to enable this test to pass, although this adds complexity to the test

ajdawson · 2015-07-15T07:50:44Z

Perhaps this was happening all along but on Python 2 the comparison just returns False? See below.

Python 3:

>>> from mock import MagicMock
>>> m = MagicMock()
>>> m > 1
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: unorderable types: MagicMock() > int()

Python 2:

>>> from mock import MagicMock
>>> m = MagicMock()
>>> m > 1
False

QuLogic · 2015-07-15T08:25:18Z

Yes, that's correct; Python 2 sorts them arbitrarily. If the actual points don't really matter, how about just setting them to 0?

QuLogic · 2015-07-18T09:36:43Z

Those metadata elements are not required for this test

I'm not sure that's true. The code from here onward has several conditions that depend on bdx and bdy, and are depending on mock's automatic attribute behaviour. But lbcode is set to select the right conditions earlier, so I think bdx and bdy should be set similarly.

rhattersley · 2015-07-30T10:39:51Z

lib/iris/tests/results/file_load/known_loaders.txt

We don't really want this list changing order ... that indicates a potential change in behaviour.

The order comes from the FormatSpecification comparitor:

def __lt__(self, other): if not isinstance(other, FormatSpecification): return NotImplemented return (-self.priority, hash(self)) < (-other.priority, hash(other))

Eww 😱

Perhaps the simplest solution is to tweak the definitions in lib/iris/fileformats/__init__.py to use unique numbers that preserve the current order?

I assumed (perhaps incorrectly) that as long as they are at the same priority, it should not matter which one is ordered first. I don't think the tests have complained so far...

Format specifications with the same priority should have mutually exclusive file matching criteria, in which case we would be able to get away with it. I'll check...

Mostly they are mutually exclusive, but sadly not in the case of the ABF/L formats vs. the other "priority 3" formats. For example, a FieldsFile named "foo.abf" would load OK using the old order, but would try to load as an ABF file using the new order.

NB. We really shouldn't be relying on consistent hash results anyway - e.g. http://bugs.python.org/issue13703.

Given the minimal impact I suggest we just accept the change as it makes the order more predictable and controllable.

rhattersley · 2015-07-30T11:02:38Z

I think bdx and bdy should be set similarly.

👍 Yes, that makes sense. Just as we don't really care about _x_coord_name when checking cell methods, we don't care about bdx, but we need to set both just to get through that routine. (You might need to set bzx, lbnpt, bzy and lbrow as well.)

FWIW, I'd set bdx and bdy to 1. If you need to set bzx and bzy I'd set them to 0. I'd set lbnpt and lbrow to 3.

Sometimes the name can be an int and sometimes a str, which are unsortable in Python 3. Emulate Python 2 behaviour for consistency.

This should allow consistent comparisons when some elements of the tuple are None. It's only used for sorting.

Again, an element may be None, so emulate Python 2 behaviour and sort that first.

This attribute is compared with 128 during parsing, but Mock is not comparable.

It doesn't make sense to use the hash of an object as a sort key since it turns out to be non-portable.

Again, an element may be None, so emulate Python 2 behaviour and sort that first.

QuLogic · 2015-09-03T05:29:40Z

Sorry for the delay; I needed to set a few of the ones you mentioned plus x_bounds=None so it wouldn't get confused setting an inconsistent bounds.

Portable comparisons (for sorting) in Py3.

QuLogic reviewed Jun 24, 2015
View reviewed changes

rhattersley reviewed Jun 25, 2015
View reviewed changes

QuLogic force-pushed the portable-sort branch 2 times, most recently from 5277a33 to 184e165 Compare June 26, 2015 08:55

QuLogic reviewed Jun 26, 2015
View reviewed changes

QuLogic force-pushed the portable-sort branch from 184e165 to eed402a Compare July 1, 2015 06:50

rhattersley added this to the v1.9 milestone Jul 30, 2015

rhattersley added the python3 label Jul 30, 2015

rhattersley reviewed Jul 30, 2015
View reviewed changes

QuLogic mentioned this pull request Jul 30, 2015

Python 3 support #1658

Closed

14 tasks

QuLogic added 6 commits September 3, 2015 01:04

py3k: Fix sorting during merge of ProtoCube.

a8315d3

Sometimes the name can be an int and sometimes a str, which are unsortable in Python 3. Emulate Python 2 behaviour for consistency.

py3k: Add comparison operator for CoordDefn.

22b6fee

This should allow consistent comparisons when some elements of the tuple are None. It's only used for sorting.

py3k: Fix sorting during CubeList merge.

2627d8e

Again, an element may be None, so emulate Python 2 behaviour and sort that first.

Set table2Version when testing GRIB edition 1.

13f7771

This attribute is compared with 128 during parsing, but Mock is not comparable.

py3k: Fix sorting of FormatSpecification.

909796b

It doesn't make sense to use the hash of an object as a sort key since it turns out to be non-portable.

py3k: Fix sorting during concatenate.

b38cd66

Again, an element may be None, so emulate Python 2 behaviour and sort that first.

Add extra fields in tests to avoid sort errors.

bc96904

QuLogic force-pushed the portable-sort branch from 4ba7cdb to bc96904 Compare September 3, 2015 05:15

pelson added a commit that referenced this pull request Sep 3, 2015

Merge pull request #1709 from QuLogic/portable-sort

06b6d20

Portable comparisons (for sorting) in Py3.

pelson merged commit 06b6d20 into SciTools:master Sep 3, 2015

QuLogic deleted the portable-sort branch September 3, 2015 08:06

Portable comparisons (for sorting) #1709

Portable comparisons (for sorting) #1709

Uh oh!

Conversation

QuLogic commented Jun 24, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

QuLogic commented Jun 24, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

QuLogic commented Jun 26, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

QuLogic commented Jul 1, 2015

Uh oh!

QuLogic commented Jul 7, 2015

Uh oh!

marqh commented Jul 14, 2015

Uh oh!

QuLogic commented Jul 14, 2015

Uh oh!

marqh commented Jul 15, 2015

Uh oh!

ajdawson commented Jul 15, 2015

Uh oh!

QuLogic commented Jul 15, 2015

Uh oh!

QuLogic commented Jul 18, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhattersley commented Jul 30, 2015

Uh oh!

QuLogic commented Sep 3, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants