-
Notifications
You must be signed in to change notification settings - Fork 298
Remove python 2 specific code #3507
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
Conversation
| return operator.div | ||
| except AttributeError: | ||
| return operator.truediv | ||
| return operator.truediv |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stephenworsley It seems that the whole point of the data_op property was convenience i.e., to do the right thing across Python2 and Python wrt div and truediv.
Now that we're Python3, I'm just questioning the need for the existence of the data_op property.
Can't we just use operator.truediv or / directly inline for the tests? i.e.,
def test_unmasked_div_zero(self):
# Ensure cube behaviour matches numpy operator behaviour for the
# handling of arrays containing 0.
dat_a = np.array([0., 0., 0., 0.])
dat_b = np.array([2., 2., 2., 2.])
cube_a = Cube(dat_a)
cube_b = Cube(dat_b)
com = dat_b / dat_a
res = self.cube_func(cube_b, cube_a).data
self.assertArrayEqual(com, res)Also drop the import operator as well 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it looks like what's going on here is that these classes are actually inheriting a bunch of tests from https://github.com/SciTools/iris/blob/master/lib/iris/tests/unit/analysis/maths/__init__.py
data_op is an abstract property in the parent class where it is used in other tests
iris/lib/iris/tests/unit/analysis/maths/__init__.py
Lines 28 to 31 in 66423f8
| @abstractproperty | |
| def data_op(self): | |
| # Define an operator to be called, I.E. 'operator.xx'. | |
| pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stephenworsley Cool.
I guess what I'm pushing is that the whole data_op property as a concept is now defunct and should be removed. For me it's simply technical debt that should be purged, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it is defunct, it allows code reuse for the tests in iris.tests.unit.maths.__init__ over multiple different operations, see also addition for example https://github.com/SciTools/iris/blob/master/lib/iris/tests/unit/analysis/maths/test_add.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, so it's generic and not specific, thanks for clarifying 👍
| return operator.div | ||
| except AttributeError: | ||
| return operator.truediv | ||
| return operator.truediv |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As the above comment.
|
@stephenworsley So is this good to go? Do you have more changes in-bound? |
|
@bjlittle Thats all I can find for the moment, If we find any others they can go in another pull request. |
|
@stephenworsley Great detective work, thanks 😄 |
As requested in #3457, Python 2 specific code has been removed.