issue_comments

@hrishikeshac in case you come back to see this: thank you for taking it so far; your code was helpful to eventually getting this feature in. And we'd of course appreciate any additional contributions.

@r-beer great—you were right to start your own PR

Alright, I have done so and changed basestrings into str, now the tests run (mostly) through locally. bash 1 failed, 6539 passed, 1952 skipped, 37 xfailed, 31 warnings in 86.34s A general question concerning collaboration on existing PRs: Should I push the changes to my fork and then create a new PR that replaces @hrishikeshac's PR or shall I push to @hrishikeshac's fork and see whether it works to update the existing PR?

Or is there another option?

PS: Permission to push to hrishikeshac:corr is denied for me.

Yeah those are pretty normal. Your suggestions look right: you can keep both on both sets and eliminate any duplicate imports in the first,

(FYI I edited your comment so it displayed properly, seems you need a line break after <details>)

Alright, I only got two merge conflicts in dataarray.py:

minor merge conflict concerning imports: 1. accessors -> accessors_td 2. broadcast has been dropped in master?

Secondly, some bigger merge conflicts concerning some of dataarray's methods, but they seem to be not in conflict with each other: 1. integrate(), unify_chunks() and map_blocks added in master 2. cov() and corr() added in corr branch 3. It seems like str = property(StringAccessor) should be at the end of dataarray's definition, for mypy reasons...

Can you please comment my suggested changes (accepting either changes from master or both, if no conflicts).

Yes 100%! Let me know if that doesn't work!

@max-sixty, thanks for the fast response!

Yeah, I get the traceback and already started diving into it. However, I assumed that @hrishikeshac's branch "corr" wasn't up-to-date.

Shall I merge changes from master or develop into corr, before looking further into the tests?

I read http://xarray.pydata.org/en/stable/contributing.html, is this identical to contributing.rst? Following those guidelines, I would use the following commands to "retrieve the changes from the master branch": git fetch upstream git rebase upstream/master

Where upstream = https://github.com/pydata/xarray.git?

@max-sixty, thanks for the fast response!

Yeah, I get the traceback and already started diving into it. However, I assumed that @hrishikeshac's branch "corr" wasn't up-to-date.

Shall I merge changes from master or develop into corr, before looking further into the tests?

Great @r-beer , we can be helpful in getting you up & running

Given this branch has diverged from master, I would make your own fork and merge in master; looks like you'll have some minor conflicts. (more details in our contributing.rst docs, or post here if confused). You can then open up your own draft PR.

Re the tests: pytest should print a list of the tests that failed and their stack traces, do you not see anything?

Dear @Hoeze, I will (try to) finalize this merge request, as I am also very interested in this functionality.

I am new to xarray and contribution. I downloaded @hrishikeshac's code and ran the pytest tests locally.

I get 17 failed, 2088 passed, 2896 skipped, 7 xfailed, 754 warnings in 49.95s.

Is there an elegant way to share "which tests failed where" in order to avoid that I try to fix tests, that might already have been fixed in other branches?

I will already start to get a better understanding of why the tests fail and try to fix them in the meantime.

Is this pull request still up to date?

I also think making this a function is probably a good idea, even though it's different from pandas.

One question: how should these functions align their arguments? Recall that xarray does an inner join for arithmetic (though there's an option to control this), and an outer join in most other cases. It's not entirely obvious to me what the right choice is here (or if it really even matters).

I always assumed an inner join is the way to go. I had initially just implemented align, but later changed to broadcast since the align doesn't add dimension/ labels (if missing in one of the inputs) to the output, but broadcast does. Without this, the where(valid_values) doesn't work if one input is 1-D and the other is N-D.

I agree that the case for DataArray.dot is questionable. It sort of makes sense because numpy and pandas both have it as a method, but the @ operator is a really a cleaner way to express this now that we're Python 3 only. (Speaking of which, why don't we support @ in xarray yet? :).)

On Mon, Jan 7, 2019 at 1:43 AM Keisuke Fujii notifications@github.com wrote:

@max-sixty https://github.com/max-sixty I am not sure whether DataArray.dot is a right choice. But I am wondering for cov case, it sounds like to compute a covariance of the DataArray itself rather than the cross covariance with another DataArray.

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

@max-sixty I am not sure whether DataArray.dot is a right choice. But I am wondering for cov case, it sounds like to compute a covariance of the DataArray itself rather than the cross covariance with another DataArray.

Probably a function like xr.cov(x, y) is better than method?

We should be more concerned with correctness than consistency - but is having DataArray.dot consistent with DataArray.cov? If not, what's the salient difference?

I also think making this a function is probably a good idea, even though it's different from pandas.

One question: how should these functions align their arguments? Recall that xarray does an inner join for arithmetic (though there's an option to control this), and an outer join in most other cases. It's not entirely obvious to me what the right choice is here (or if it really even matters).

I agree with @fujisoup

I am a little worrying that users could misunderstand cov is for (auto-)covariance rather than cross-covariance, which we are implementing here. Probably a function like xr.cov(x, y) is better than method?

Made the code PEP8 compatible. Apologies for not doing so earlier.

Hello @hrishikeshac! Thanks for updating the PR.

• In the file xarray/core/dataarray.py, following are the PEP8 issues :

Line 2417:80: E501 line too long (85 > 79 characters) Line 2448:80: E501 line too long (86 > 79 characters)

• In the file xarray/tests/test_dataarray.py, following are the PEP8 issues :

Line 3312:80: E501 line too long (92 > 79 characters) Line 3313:80: E501 line too long (89 > 79 characters) Line 3342:80: E501 line too long (100 > 79 characters)

Comment last updated on January 04, 2019 at 23:39 Hours UTC