Deprecate CubeSlice shape property

[slobodan-ilic]

• Deprecation warning when shape property is used
• Implement get_shape method, that uses prune argument
• Unit tests

[coveralls]

Coverage increased (+0.1%) to 98.468% when pulling c59ecbb on prune-slice-shape-160788065 into 20df047 on master.

[scanny]

Please add docstring (PEP 257) describing interface and what a shape is.

[slobodan-ilic]

Done. I've run the pylint on the entire file, should be better now...

[percious]

memoize this.

[scanny]

It would always be preferable to avoid re-using the name of the method in its docstring. If a person doesn't know what "shape" means in this context, this docstring is going to make them work for it. So in this case, the second sentence might be better as: "It returns a tuple of ints, each representing the length of a cube dimension, in the order those dimensions appear in the cube."

I would add a blank line before the deprecated sentences. That topic is separate enough to warrant a distinct paragraph.

[slobodan-ilic]

Thanks for the thoughtful comment. This level of code review really means much to me.

I've addressed the comment. The first line of the docstring I copied from here, and modified it a little bit. The second sentence I took as you put it.

Please also check if the other modified docstring (of the method get_shape) is OK.

[scanny]

Okay, I left some feedback for whatever it's worth to you, but changes are not required, just some suggestions for possible improvements.

[scanny]

This looks like a good candidate for a parameterized test, maybe something like this:

def it_knows_its_shape(self, shape_fixture):  # ---or named `test_shape()` if you prefer---
    cube_slice, prune, expected_shape = shape_fixture
    shape = cube_slice.get_shape(prune=prune)
    assert shape == expected_value

@pytest.fixture(params=[
    (False, None, (3, 2)),
    (True, [[True, False], [True, False], [True, False]], (3,)),
    etc.
])
def shape_fixture(self, request, cube_):
    prune, mask_map, expected_value = request.param
    cube_.ndim = 2
    cube_.as_array.return_value = np.ma.masked_array(
        np.zeros((3, 2)), mask=np.array(mask_map) if mask_map else None
    )
    cube_slice = CubeSlice(cube_, 0)
    return cube_slice_, prune, expected_value

This isn't complete (or even correct) in every detail, but maybe gives you an idea. I recommend making the warning test a separate test method since its form and asserts are very different.

Using the fixture allows the test to be very clear and adding new cases is trivial and doesn't duplicate code.

[scanny]

Sweet ❤️ :)

[scanny]

It's a fine point, but I always use the word "Return" as the first word of the docstring for a method that returns a value. So something like "Return scale-means for this cube slice." This is the key aspect of the contract of such a method (the return value and type) and what the caller cares about most. Saying "Calculate" could just indicate a side-effect and leaves the reader to dig into the code (which often they won't have when just looking at the docs).

As an aside, in the case of a property, I leave the 'Return' word out, like "int count of foos in bar" instead of 'Return int count of foos in bar', since the semantics of a property are a value rather than a procedure.

You'll also need to say what scale-means are and what form you deliver them in. If that's not compact enough for the summary line, then in a body paragraph of the docstring. Even then, it's worth every effort not to repeat the method name in the summary because it provides no additional information. It could be something like """Return list containing scaled-mean for each slice column.""" or something and already starts to give more insight into the method contract.

[slobodan-ilic]

Done, thanks