Maximize coverage #542
Maximize coverage #542
Conversation
| else: | ||
| # deal with change dims->shape in arguments as of numpy 1.16 | ||
| self.chunk_mixs = np.unravel_index(self.chunk_rixs, shape=array._cdata_shape) | ||
| self.chunk_mixs = np.unravel_index(self.chunk_rixs, array._cdata_shape) |
There was a problem hiding this comment.
Do you know if the keyword argument was needed for some reason @alimanfoo? Noticed this change came from here. Hence the question 🙂
There was a problem hiding this comment.
In general, I think using kwargs is better when it's defined as a kwarg rather than relying on the order of positional args. The previous block looked like it was here to deal with numpy <1.16 and numpy >1.16 but in our setup.py we specify numpy>=1.17, so I think it's safe to drop handling old numpy versions and use shape= but not fussed too much either way.
There was a problem hiding this comment.
IIUC our setup.py has NumPy 1.7 as the minimum supported version
Lines 12 to 17 in 3ae9439
There was a problem hiding this comment.
So I guess we do need to consider older versions of NumPy where the signature keywords may have been different
There was a problem hiding this comment.
Ah, I kept reading 1.7 as 1.17... 🤦♂
There was a problem hiding this comment.
Seems to be largely unchanged over time. 🙂 Here are the docs from NumPy 1.7.
There was a problem hiding this comment.
Cool, cool. Happy for this to be merged on my side :)
| else: | ||
| # deal with change dims->shape in arguments as of numpy 1.16 | ||
| self.chunk_mixs = np.unravel_index(self.chunk_rixs, shape=array._cdata_shape) | ||
| self.chunk_mixs = np.unravel_index(self.chunk_rixs, array._cdata_shape) |
There was a problem hiding this comment.
In general, I think using kwargs is better when it's defined as a kwarg rather than relying on the order of positional args. The previous block looked like it was here to deal with numpy <1.16 and numpy >1.16 but in our setup.py we specify numpy>=1.17, so I think it's safe to drop handling old numpy versions and use shape= but not fussed too much either way.
|
Going to go ahead and merge. Guessing Alistair is busy with other things and the risk here is low. Though if that's not true, please raise an issue and we can work to address it. Thanks all! 😄 |
These were uncovered lines in this Coveralls report. As we already dropped Python 2 in PR ( #470 ), go ahead and drop this no longer need Python 2 code.
Also smooth over differences in
unravel_index's signature across NumPy versions.