issue_comments

BTW, I think I messed up my co-worker's name in whats-new.rst. The spelling is right (Kyle Heuton), but the extra indentation shouldn't be there. As a result his name appears a bit out of place. Would be very much obliged if you could fix that in your next push to master. Gracias.

No worries, whats-new.rst usually requires a fix-up before each release

Thank you very much for your patience!

BTW, I think I messed up my co-worker's name in whats-new.rst. The spelling is right (Kyle Heuton), but the extra indentation shouldn't be there. As a result his name appears a bit out of place. Would be very much obliged if you could fix that in your next push to master. Gracias.

api.rst should've been a checklist item :)

Indeed, I will add it right now!

Thanks for your contribution!!

api.rst should've been a checklist item :)

One last thing -- this needs an entry in docs/api.rst in order for the doc page to get built.

If that's the case then I have nothing more to add. I've run flake8 on the individual files I touched I as well, such as flake8 xarray/core/dataset.py, and none of my changes are flagged.

It's very strange that fake8 complains about non-Python files. I think you can safely ignore those. On Wed, May 10, 2017 at 8:55 PM chunweiyuan notifications@github.com wrote:

git diff upstream/master | flake8 --diff only complains about a bunch of lines in whats-new.rst and reshaping.rst. But these complaints don't make any sense to me. Some examples:

doc/reshaping.rst:195:1: E112 expected an indented block doc/reshaping.rst:203:1: E101 indentation contains mixed spaces and tabs doc/reshaping.rst:203:1: W191 indentation contains tabs doc/reshaping.rst:203:2: E113 unexpected indentation

I've played around with it a bit but not seen any changes to the complaints. What is going on? Should I even worry about the .rst files?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/pydata/xarray/pull/1389#issuecomment-300675689, or mute the thread https://github.com/notifications/unsubscribe-auth/ABKS1qjB4dK0tHLBMrX6sURnlt_T5QX0ks5r4obPgaJpZM4NMGMU .

git diff upstream/master | flake8 --diff only complains about a bunch of lines in whats-new.rst and reshaping.rst. But these complaints don't make any sense to me. Some examples: doc/reshaping.rst:195:1: E112 expected an indented block doc/reshaping.rst:203:1: E101 indentation contains mixed spaces and tabs doc/reshaping.rst:203:1: W191 indentation contains tabs doc/reshaping.rst:203:2: E113 unexpected indentation I've played around with it a bit but not seen any changes to the complaints. What is going on? Should I even worry about the .rst files?

Haha, brilliant.

Scroll to the top of this PR and look at your first post :)

On Wed, May 10, 2017 at 11:08 AM, chunweiyuan notifications@github.com wrote:

I just made some futile attempts to find that checklist, to no avail. Mind helping me with a link? :)

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/pydata/xarray/pull/1389#issuecomment-300566860, or mute the thread https://github.com/notifications/unsubscribe-auth/ABKS1mF7Jjb2NmZ_5LZpRvUP_nwG-khgks5r4f0hgaJpZM4NMGMU .

I just made some futile attempts to find that checklist, to no avail. Mind helping me with a link? :)

Today is the first time I've heard of flake8.

It's in the checklist for every new PR :).

Is this how you guys standardize quality checks?

Yes. It would be nice to run this as a continuous integration test but we haven't set that up that.

Running it once with either Python 2 or 3 is fine -- it should give the same output either way.

Today is the first time I've heard of flake8. Is this how you guys standardize quality checks? If so, do you run once for python 3.5 and once for 2.7?

@chunweiyuan have you run flake8 on this?

Looks ready now.

Think I just copied something over from variable.py and changed the message a little bit...

Ah, good to know.

In that case, let's add a check for dtype == object based on numpy.__version__ (search for LooseVersion for examples in the xarray codebase) and raise an informative NotImplementedError when necessary.

From numpy 1.11.0 release notes: ``` np.lexsort now supports arrays with object data-type

The function now internally calls the generic npy_amergesort when the type does not implement a merge-sort kind of argsort method. ```

1.10.4 failed with the merge sort error. 1.11.0 failed with some error that had nothing to do with merge sort (Reason: image not found. Make sure your test modules/packages have valid Python names.) Starting from 1.11.1 it works fine.

What's the best course of action now?

If I downgrade to numpy 1.10, I get that error. Once upgraded to 1.12, it goes away......

Does it work on numpy 1.11?

We could potentially (partially) drop compatibility for older numpy releases. Or if we can identify the issue, we can raise an informative error or use a work-around.

If I downgrade to numpy 1.10, I get that error. Once upgraded to 1.12, it goes away......

Looks like one the Travis-CI tests failing on the sortby tests: https://travis-ci.org/pydata/xarray/jobs/229457729#L342

It's not immediately clear to me what's going on there, but maybe it's an issue with the old version of NumPy? The error is TypeError: merge sort not available for item 0.

Perhaps it's finally ready for prime time?

I've added a tiny bit of extra docstring just to ease my discomfort about the NaN sort. If you feel it's redundant I can remove it.

Almost time to merge this baby in? :)

Indeed, please ignore the first CI build on Travis. For some inexplicable reason it is installing the pandas 0.20rc, which is failing due to https://github.com/pydata/xarray/issues/1386.

I am slightly in favor of sortby but also could go either way. The PEP8 guidance on function/method naming is "lowercase with words separated by underscores as necessary to improve readability." I don't think an underscore is necessary for readability here.

Weird. Travis seems to fail in places unrelated to my changes.

I'm rather agnostic to sortby vs. sort_by.

We want this as sortby, rather than sort_by? Generally the latter would be more idiomatic, but if there is prior sortby out there, I would vote for consistency