Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix assignment with .loc #8067

Merged
merged 10 commits into from
Sep 12, 2023
Merged

Fix assignment with .loc #8067

merged 10 commits into from
Sep 12, 2023

Conversation

dranjan
Copy link
Contributor

@dranjan dranjan commented Aug 13, 2023

Apparently DataArray instances on the right-hand side of Variable.__setitem__ were being stripped of their xarray metadata, leading to the incorrect broadcasting noted in #7030. My proposed fix is to add an explicit if-clause for this case.

@welcome
Copy link

welcome bot commented Aug 13, 2023

Thank you for opening this pull request! It may take us a few days to respond here, so thank you for being patient.
If you have questions, some answers may be found in our contributing guidelines.

@dcherian dcherian changed the title Fix for #7030 Fix assignment with .loc Aug 14, 2023
@dranjan
Copy link
Contributor Author

dranjan commented Aug 16, 2023

I realized that I hadn't paid much attention to Dataset vectorized assignment here. I took a look at the code and I think the fix implemented here should also work there, but it's probably worth adding a regression test for Dataset as well for the #7030 bug. That said, I don't want to hold up reviewing and merging this pull request, so I can do that in a separate small follow-up PR, or just put it here if that works. I would probably be getting to it later today after I leave work.

(By the way, I'm not sure what's happening with the Mypy CI checks, but I think it's not from this PR, because other recent PRs are also having the same failure.)

@dranjan
Copy link
Contributor Author

dranjan commented Aug 18, 2023

I went ahead and added the test for Dataset. I figure putting it in this PR is less work for everyone.

Copy link
Collaborator

@max-sixty max-sixty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a look through this, it looks good. Thanks @dranjan

I've been away for a while so want to be conservative about merging things myself — I'll see if anyone else comments, otherwise let's merge?

(and sorry for the delay)

@max-sixty max-sixty added the plan to merge Final call for comments label Sep 10, 2023
@dranjan
Copy link
Contributor Author

dranjan commented Sep 11, 2023

Awesome, I'm glad to see progress on this! Do you need anything more from me, like resolving that minor merge conflict?

@max-sixty
Copy link
Collaborator

I fixed the merge conflict and will merge. (If anyone more informed disagrees, we can revert in the worst case.)

Thank you very much @dranjan !

@max-sixty max-sixty enabled auto-merge (squash) September 11, 2023 22:19
@max-sixty max-sixty disabled auto-merge September 12, 2023 18:16
@max-sixty max-sixty merged commit 8215911 into pydata:main Sep 12, 2023
@welcome
Copy link

welcome bot commented Sep 12, 2023

Congratulations on completing your first pull request! Welcome to Xarray! We are proud of you, and hope to see you again! celebration gif

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plan to merge Final call for comments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Assigning with .loc indexing: implicit effect of dimension order?
3 participants