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.

Sign up for GitHub

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

Jump to bottom

Fix master np113 #2616

Merged
merged 5 commits into from
Jun 24, 2017
Merged

Fix master np113 #2616

merged 5 commits into from
Jun 24, 2017

Conversation

bjlittle
Copy link
Member

@bjlittle bjlittle commented Jun 22, 2017 •
edited
Loading

This PR resolves the current travis-ci issues for master, which arise from the latest release of numpy, which bumped from v1.12.1 to v1.13.0.

The issues are related to:

  • each output nD coordinate dtype produced by numpy.meshgrid is now the same as its associated 1D input coordinate dtype
  • a change within numpy.core.arrayprint.array2string means that we require to be more general with the formatter option i.e. numpystr -> all
  • correcting a data_manager unit test

@bjlittle bjlittle mentioned this pull request Jun 22, 2017
Merged
@bjlittle
Copy link
Member Author

What the heck is going on with travis ... sigh 😢

@bjlittle
Copy link
Member Author

Ping @SciTools/iris-devs ... anyone fancy taking this on?

All PRs on master are broken until this gets merged ...

@QuLogic
Copy link
Member

QuLogic commented Jun 22, 2017

But, it... passes?

@bjlittle
Copy link
Member Author

@QuLogic For example, this open PR #2614 is failing, see it's travis-ci build ... this PR will resolve those issues.

@pp-mo
Copy link
Member

pp-mo commented Jun 23, 2017

I see that, outside the tests, we still have just a couple of usages of the original meshgrid in iris.plot._map_common

Is there a particular reason for that, or did it just get overlooked ?

Obviously it's making no difference at this point, but I guess that would be true of some others.
I can understand why you don't bother changing all the test usages, if it rocks no boats. I guess that makes good sense.

pp-mo
pp-mo reviewed Jun 23, 2017
View reviewed changes
lib/iris/__init__.py Outdated
@@ -122,7 +122,7 @@ def callback(cube, field, filename):


# Iris revision.
__version__ = '2.0.0-DEV'
__version__ = '2.0.dev0'
Copy link
Member

Choose a reason for hiding this comment

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

Can you maybe explain exactly what this is about, just for future reference ?

Copy link
Member Author

Choose a reason for hiding this comment

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

To be fair, this doesn't belong in this PR, so I'll pull it ... but the __version__ should be PEP440.

I'll save that for another day and another PR 👍

pp-mo
pp-mo reviewed Jun 23, 2017
View reviewed changes
lib/iris/tests/results/analysis/rotated_pole.0.rlat.json
{"std": 3.7195863723754883, "min": -5.5932002067565918, "max": 8.7213001251220703, "shape": [93, 75], "masked": false, "mean": 1.6348202228546143}
Copy link
Member

@pp-mo pp-mo Jun 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

It might have been "nicer" here to reduce the tolerance on the test for when it happens again (!).
I see these values only just fail the test at a relative tolerance of 1e-7, as default for assertArrayAllClose (+hence assertDataAlmostEqual) : mean value relative change is approx 1.22e-7.
You could maybe add something like , reltol=5.0e-6 to the 4 calls here
You could do that instead, or as well if you prefer (changes are very small after all).

Just a suggestion -- skip if you don't want.

Copy link
Member Author

Choose a reason for hiding this comment

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

@pp-mo I'm really against fudging the tolerances. We took that step previously with testing and it ultimately lead to a bad place, particularly with graphical testing.

So, I'd rather not take that option, unless you consider it unwise.

Copy link
Member

Choose a reason for hiding this comment

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

I don't consider it fudging as long as the value used remains appropriate.
I know that was not always the case for our image comparison tolerance, where it got stupidly huge for a while... !
The default value of 1e-7 is designed to cope with float32 rounding errors, but can easily be a bit fine for testing other calculated values, like coordinate projections.
For instance, I just encountered some other tests failing at that level when I was messing about with different dask chunkings.

pp-mo
pp-mo reviewed Jun 23, 2017
View reviewed changes
lib/iris/analysis/cartography.py Outdated
@@ -239,6 +239,24 @@ def _xy_range(cube, mode=None):
return (x_range, y_range)


def _meshgrid(x, y):
Copy link
Member

Choose a reason for hiding this comment

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

I think on balance I'd prefer this to live in iris.util, but only slightly -- your call.
Please do keep it private !

Copy link

Choose a reason for hiding this comment

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

Agreed. Is this not behaviour that would be useful to have consistent across the entire package?

Perhaps if util were made a sub-package, then it would no longer be such a difficult call to add add additional 'utilities' for fear of further discontinuity:

iris.util.ndarray.meshgrid

In ANTS we have the following structure, which has served us very well:

ants.utils.ndarray
ants.utils.coord
ants.utils.cube

Copy link
Member

Choose a reason for hiding this comment

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

Not looked at the details, but numpy/numpy#5302 refers.

@pp-mo
Copy link
Member

pp-mo commented Jun 23, 2017

A couple of piffling suggestions : I should really have grouped them into a review, but now it's too late.

No show-stoppers, I am fully 👍 + happy to merge on your response.

pelson
pelson approved these changes Jun 23, 2017
View reviewed changes
Copy link
Member

@pelson pelson left a comment

Choose a reason for hiding this comment

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

I question whether we should be (effectively) mocking out meshgrid's behaviour and whether the numpy change was done intentionally - I'd be interested in having that chased up. In the meantime, 👍.

lib/iris/analysis/cartography.py Outdated
@@ -239,6 +239,24 @@ def _xy_range(cube, mode=None):
return (x_range, y_range)


def _meshgrid(x, y):
Copy link
Member

Choose a reason for hiding this comment

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

Not looked at the details, but numpy/numpy#5302 refers.

@bjlittle
Copy link
Member Author

@pp-mo and @pelson I've addressed the review actions, so I think this PR is good to go ...

This was referenced Jun 23, 2017
Merged
Merged
Merged
@pelson pelson merged commit ceaddb5 into SciTools:master Jun 24, 2017
@bjlittle bjlittle deleted the fix-master-np113 branch June 25, 2017 22:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pp-mo pp-mo pp-mo left review comments

@cpelley cpelley cpelley left review comments

@pelson pelson pelson approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@bjlittle @QuLogic @pp-mo @pelson @cpelley