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.

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

Make cube.exponentiate lazy #1846

Closed
wants to merge 4 commits into from
Closed

Conversation

DPeterK
Copy link
Member

@DPeterK DPeterK commented Nov 19, 2015

Changes the last line in iris.analysis.maths._math_op_common to use lazy data. This means iris.analysis.maths.exponentiate (providing cube ** exponent functionality) is now lazy.

As I was in the space, I've also added some tests for iris.analysis.maths.exponentiate. These do not follow the pattern of the other test modules in the directory as we do not make use of an operator attribute for cube exponentiation.

test_data = tests.get_data_path(('PP', 'simple_pp', 'global.pp'))
cube = iris.load_cube(test_data)
exponentiate(cube, self.exponent, in_place=True)
self.assertTrue(cube.has_lazy_data())
Copy link
Member

Choose a reason for hiding this comment

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

You missed the crucial step here. If you checked the data, I think you will find that it isn't correct.

@DPeterK
Copy link
Member Author

DPeterK commented Nov 20, 2015

Review actions. I've simplified maths._math_op_common and got maths.exponentiate using biggus.power. I've also updated the tests with more coverage. Note that the masked array test is fixed by biggus #167.

result = exponentiate(cube, self.exponent)
self.assertMaskedArrayEqual(result.data, expected)

@tests.skip_data
Copy link
Member

Choose a reason for hiding this comment

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

We don't really need to use real data here. We can just use the lazy_array method to get lazy data on the cube.

@pelson
Copy link
Member

pelson commented Nov 23, 2015

I think this is getting there. Is there documentation which needs updating? Is there in_place docs somewhere which describe the behaviour?

@DPeterK
Copy link
Member Author

DPeterK commented Nov 23, 2015

Is there documentation which [sic] needs updating?

I've taken a look; there is nothing in the cube mathematics section of the userguide about performing operations inplace. In the reference guide almost all of the mathematical operators have the in_place kwarg described as follows:

    in_place:
        Whether to create a new Cube, or alter the given “cube”.

It's pretty terse, which actually means it is still correct. I could update it:

    in_place:
        Whether to create a new Cube as a copy of the given "cube" and alter its data to be
        the result of applying the mathematical operation, or alter the data of the given
        “cube” to be the result of applying the mathematical operation.

But that's increased the length of the description, which has not necessarily helped its cause.

@pelson
Copy link
Member

pelson commented Nov 23, 2015

But that's increased the length of the description, which has not necessarily helped its cause.

Agree, that doesn't really help. Maybe just a what's new item then.

@pp-mo
Copy link
Member

pp-mo commented Nov 23, 2015

Is there documentation which needs updating?

++ whatsnew ??

@pelson
Copy link
Member

pelson commented Nov 23, 2015

@pp-mo - is this lined for 1.9?

@pp-mo
Copy link
Member

pp-mo commented Nov 24, 2015

@pelson pp-mo - is this lined for 1.9?

Not unless you want to push for it.
We will have lazy basic arithmetic (+-*/) covered in the 1.9 whatsnew, though

@DPeterK
Copy link
Member Author

DPeterK commented Jun 15, 2016

Rebase. Wonder whether the Biggus updates between when I opened this and now will have helped move this along?

@corinnebosley
Copy link
Member

@dkillick Is there anything that can be done with this to get it moving again? The ticket is a year old. That's a bit ridiculous. Let me know if I can do something about this or bother someone who can.

@DPeterK
Copy link
Member Author

DPeterK commented Nov 11, 2016

Another six months, another rebase... thanks @corinnebosley

@corinnebosley
Copy link
Member

@dkillick Well that sucks.

@corinnebosley
Copy link
Member

@dkillick 'The build has been terminated'.... Must be because it's Friday afternoon. Travis is probably at the pub by now.

@@ -0,0 +1,94 @@
# (C) British Crown Copyright 2015, Met Office
Copy link
Member

Choose a reason for hiding this comment

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

If the license header is the only problem, we might be ok after all.

Copy link
Member Author

Choose a reason for hiding this comment

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

That is far from being our only problem I suspect... Is it really complaining about the license header??

Copy link
Member

Choose a reason for hiding this comment

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

So far, but the tests didn't complete, so...

@QuLogic
Copy link
Member

QuLogic commented Nov 12, 2016

I'd say see #2229.

@DPeterK
Copy link
Member Author

DPeterK commented Nov 14, 2016

@QuLogic this (and #2229) may be helped by #2240. I'll restart the build to see if that's the case.

@DPeterK
Copy link
Member Author

DPeterK commented Nov 14, 2016

Except, of course, we can't make use of the test runner changes in #2240 until the mergeback (#2236) is, well, merged back...

@marqh
Copy link
Member

marqh commented Nov 16, 2016

Except, of course, we can't make use of the test runner changes in #2240 until the mergeback (#2236) is, well, merged back...

@dkillick this is now merged, so if you would be knid enough to re-rebase, that would be helpful

@DPeterK
Copy link
Member Author

DPeterK commented Nov 16, 2016

if you would be knid enough to re-rebase

Done (and mercifully painless!) - thanks @marqh

@marqh
Copy link
Member

marqh commented Nov 21, 2016

there is now an explicit text failure that needs addressing:
FAIL: test_square_root (iris.tests.test_basic_maths.TestExponentiate)

@marqh
Copy link
Member

marqh commented Jan 3, 2017

Hi @dkillick

please could you review the test failures and see if this PR can be brought up to speed?

If successful, please consider targeting the 1.12.x branch for 1.12 release

thank you
mark

@pelson
Copy link
Member

pelson commented Oct 30, 2017

This PR is probably quite out of date with the recent dask work for v2. It might be worth checking to see if we have already go exp to be lazy at this point...

@pelson pelson closed this Oct 30, 2017
@pelson
Copy link
Member

pelson commented Oct 30, 2017

Update: it's ambiguous as to whether it is lazy, but there is certainly scope for it to be lazy with the current implementation. https://github.com/SciTools/iris/blob/master/lib/iris/analysis/maths.py#L542 has the line:

return operator.pow(data, exponent)

Which presumably means that if data is a dask array, and it has lazy exp, then iris does lazy exp... 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants