Skip to content

Conversation

@ianhi
Copy link
Contributor

@ianhi ianhi commented Dec 12, 2025

@ianhi ianhi changed the title add drop_existing kwarg to set_xindex add drop_existing kwarg to set_xindex Dec 12, 2025
@dcherian dcherian requested a review from benbovy December 12, 2025 18:31
Copy link
Member

@benbovy benbovy left a comment

Choose a reason for hiding this comment

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

Thanks @ianhi, I think that this is a convenient addition!

I'd just rename drop_existing to simply drop. The latter is a bit less self-explanatory but more consistent with the rest of the Xarray API where drop is already a parameter of many methods like .reset_index, .reset_coords, .isel, .where, etc.

@max-sixty
Copy link
Collaborator

just to confirm — and I left a comment at #11007, so possibly folks saw this — we're sure this shouldn't replace by default? currently the closest function is set_index, which does exactly that, iiuc

@benbovy
Copy link
Member

benbovy commented Dec 16, 2025

we're sure this shouldn't replace by default? currently the closest function is set_index, which does exactly that, iiuc

I don't have strong opinions on this, we could replace it by default indeed. It might break the workflow of some users who rely on explicit-only index replacement as a safety guard, however to my knowledge people are actually rather annoyed by that (including me :-)).

@ianhi
Copy link
Contributor Author

ianhi commented Dec 16, 2025

re the default I was defaulting to being cautious given xarray's large userbase. But I think it's in general a large user base, perhaps not for this specific function. I agree that a default of drop=True is preferable. So the quesiton becomes are we comfortable with just doing that, or does that require a deprecation cycle?

@benbovy
Copy link
Member

benbovy commented Dec 16, 2025

So the quesiton becomes are we comfortable with just doing that

Yes I think we are!

@max-sixty
Copy link
Collaborator

(this might be going too far, but we may not even need the drop kwarg...)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add new kwarg drop_existing to set_xindex

3 participants