Skip to content

Remove usages of replace() method #2708

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
Merged
bjlittle merged 2 commits into from
Aug 3, 2017
Merged

Remove usages of replace() method #2708

bjlittle merged 2 commits into from
Aug 3, 2017

Conversation

@lbdreyer
Copy link
Member

@lbdreyer lbdreyer commented Aug 2, 2017

Fixes #2705

@bjlittle bjlittle self-assigned this Aug 2, 2017
lbdreyer
lbdreyer commented Aug 2, 2017
View reviewed changes
lib/iris/tests/test_basic_maths.py Outdated
self.cube = iris.tests.stock.global_pp()
self.cube.replace(self.cube.data - 260,
fill_value=self.cube.fill_value)
self.cube.data = self.cube.data - 260)
Copy link
Member Author

@lbdreyer lbdreyer Aug 2, 2017

Choose a reason for hiding this comment

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

I'll remove this bracket!

bjlittle
bjlittle reviewed Aug 2, 2017
View reviewed changes
lib/iris/tests/unit/analysis/regrid/test_RectilinearRegridder.py Outdated
src = self.src
src.coord('longitude').circular = True
src.replace(ma.MaskedArray(src.data), fill_value=src.fill_value)
src.data = ma.MaskedArray(src.data, fill_value=src.fill_value)
Copy link
Member

@bjlittle bjlittle Aug 2, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@lbdreyer Are there two spaces in src.data^^= ?

Copy link
Member Author

@lbdreyer lbdreyer Aug 2, 2017

Choose a reason for hiding this comment

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

Sorry, now fixed.

@bjlittle
Copy link
Member

bjlittle commented Aug 2, 2017

@lbdreyer All looks good. For #2706 I'm trying to retain some of the test tests, if I can, and they make sense.

I'm assuming that all the replace tests can just be purged?

@lbdreyer
Copy link
Member Author

lbdreyer commented Aug 2, 2017

I'm assuming that all the replace tests can just be purged?

Which tests are you referring to?

@bjlittle
Copy link
Member

bjlittle commented Aug 2, 2017

Which tests are you referring to?

These https://github.com/SciTools/iris/pull/2708/files#diff-1a002eee61524000a58a8e337597c7d7L1262 ...

@lbdreyer
Copy link
Member Author

lbdreyer commented Aug 2, 2017

The replace methods on iris.cube.Cube and iris._data_manager.DataManager, which those tests were testing, no longer exists so we should just delete them

@bjlittle
Copy link
Member

bjlittle commented Aug 2, 2017

One thing we did do was increase test coverage thru replacing biggus with dask.

For me replace is kinda equivalent to cube.data = data ... so I was just wondering whether there are any of those tests mentioned above, that could be re-purposed as such cube.data = data tests and live else where in another appropriate test class ... the answer is more likely not, but just thought I'd ask.

@lbdreyer
Copy link
Member Author

lbdreyer commented Aug 2, 2017

@bjlittle
I now understand your point. I think it would be overkill to keep some cube.replace tests but possibly reworking the data_manager.replace tests might make sense

bjlittle
bjlittle reviewed Aug 2, 2017
View reviewed changes
lib/iris/tests/unit/analysis/regrid/test_RectilinearRegridder.py Outdated
src = self.src
src.coord('longitude').circular = True
src.replace(ma.MaskedArray(src.data), fill_value=src.fill_value)
src.data = ma.MaskedArray(src.data, fill_value=src.fill_value)
Copy link
Member

@bjlittle bjlittle Aug 2, 2017

Choose a reason for hiding this comment

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

@lbdreyer But there is no src.fill_value ...

Copy link
Member Author

@lbdreyer lbdreyer Aug 3, 2017

Choose a reason for hiding this comment

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

In this test src was loaded in from a pp file, so the src.fill_value would presumably have been the bmdi value.

However, this test doesn't really care about the fill value so I have just let it use the numpy default fill_value

@QuLogic QuLogic added this to the dask-mask milestone Aug 2, 2017
@bjlittle
Copy link
Member

bjlittle commented Aug 3, 2017

@lbdreyer Thanks, this looks good to me 👍

@bjlittle bjlittle merged commit f5d02d9 into SciTools:dask_mask_array Aug 3, 2017
@lbdreyer lbdreyer deleted the dask_replace_rm branch July 23, 2018 10:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bjlittle bjlittle bjlittle left review comments

Assignees

@bjlittle bjlittle

Labels

None yet

Projects

None yet

Milestone

dask-mask

Development

Successfully merging this pull request may close these issues.

3 participants

@lbdreyer @bjlittle @QuLogic