Accept int value in head, thin and tail

[griverat]

Related #3278
This PR makes the methods head, thin and tail for both DataArray and Dataset accept a single integer value as a parameter. If no parameter is given, then it defaults to 5.

• Tests added
• Passes black . && mypy . && flake8

[max-sixty]

This is great! Thanks @DangoMelon

I'll wait for a day in case of other comments

[shoyer]

Does it make sense to thin by a factor of five by default? Or should we not have a default value? The use case for a default thinning value are less clear to me than defaults for head/tail.

[max-sixty]

Agree!

[dcherian]

I think picking a default makes it very convenient to use. And this is a convenience method...

[griverat]

I set a default value to thin to make it even with the other two methods (really didn't think that much of it usage) but I agree with @dcherian that it might be ok to have it for convenience.

[max-sixty]

I don't have a strong enough view. I agree it's a convenience method, but a convenient value significantly depends on the size of the array, unlike with head & tail

So whatever you think. I'm probably a -0.2

[max-sixty]

Very thorough tests! Thank you!

[max-sixty]

Any other feedback before we merge? (Errors are unrelated)

[shoyer]

It is help to include a little more context in error messages if possible. In this case, you could include offending the name and value.

[griverat]

Hmmm, Something along these lines maybe?

"expected integer as indexer value, found type %r for dim %r" % (type(v), k)

and

"expected positive integer as indexer value for dim %r" % k

The k and v come from iterating over indexers.items()

[shoyer]

Looks good to me, though it would be nice to add more context to the error messages.

[griverat]

Any other feedback before we merge? (Errors are unrelated)

@max-sixty I think the typing errors are due to changing the Any to int and actually finding slice.

[shoyer]

I would try to include value in the message, too, which can often be a useful hint about what went wrong, e.g., “expected positive integer for dimension ‘x’, got -1”

…

On Fri, Sep 13, 2019 at 8:56 PM Gerardo Rivera ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In xarray/core/dataset.py <#3298 (comment)>: > indexers = either_dict_or_kwargs(indexers, indexers_kwargs, "head") + for v in indexers.values(): + if not isinstance(v, int): + raise TypeError("indexer value must be an integer") + elif v < 0: + raise ValueError("indexer value must be positive") Hmmm, Something along these lines maybe? "expected integer as indexer value, found type %r for dim %r" % (type(v), k) and "expected positive integer as indexer value for dim %r" % k The k and v come from iterating over indexers.items() — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#3298?email_source=notifications&email_token=AAJJFVVWFOWK5LYL33Z6Q4DQJROHDA5CNFSM4IVADGGKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCEXTGGA#discussion_r324411117>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJJFVRJWPWIG5XBDWEPRMLQJROHDANCNFSM4IVADGGA> .

[max-sixty]

Here's a solution to the mypy issue:

diff --git a/xarray/core/dataset.py b/xarray/core/dataset.py
index 1d762114..ee4ebf4d 100644
--- a/xarray/core/dataset.py
+++ b/xarray/core/dataset.py
@@ -2046,8 +2046,8 @@ class Dataset(Mapping, ImplementsDatasetReduce, DataWithCoords):
                 raise TypeError("indexer value must be an integer")
             elif v < 0:
                 raise ValueError("indexer value must be positive")
-        indexers = {k: slice(val) for k, val in indexers.items()}
-        return self.isel(indexers)
+        indexers_slices = {k: slice(val) for k, val in indexers.items()}
+        return self.isel(indexers_slices)
 
     def tail(
         self,
@@ -2087,11 +2087,11 @@ class Dataset(Mapping, ImplementsDatasetReduce, DataWithCoords):
                 raise TypeError("indexer value must be an integer")
             elif v < 0:
                 raise ValueError("indexer value must be positive")
-        indexers = {
+        indexers_slices = {
             k: slice(-val, None) if val != 0 else slice(val)
             for k, val in indexers.items()
         }
-        return self.isel(indexers)
+        return self.isel(indexers_slices)
 
     def thin(
         self,
@@ -2134,8 +2134,8 @@ class Dataset(Mapping, ImplementsDatasetReduce, DataWithCoords):
                 raise ValueError("indexer value must be positive")
             elif v == 0:
                 raise ValueError("step cannot be zero")
-        indexers = {k: slice(None, None, val) for k, val in indexers.items()}
-        return self.isel(indexers)
+        indexers_slices = {k: slice(None, None, val) for k, val in indexers.items()}
+        return self.isel(indexers_slices)
 
     def broadcast_like(
         self, other: Union["Dataset", "DataArray"], exclude: Iterable[Hashable] = None

[griverat]

@shoyer I hope it's much clear now, I tried to phrase what you suggested.
@max-sixty That did the trick!
Thank you both for your suggestions.

[shoyer]

thanks @DangoMelon !

[jhamman]

@DangoMelon - thanks for your contribution. This will get used a lot!