Add a .drop_attrs method

[max-sixty]

Part of #3891

Do we think this is a good idea? I'll add docs & tests if so...

Ready to go, just needs agreement on whether it's good

[headtr1ck]

I think it's a good idea.

But should probably have the same arguments and workings as drop_vars and others.

If you want to control the behavior of dropping attrs from variables or only dataset level attrs is another question.

[max-sixty]

If you want to control the behavior of dropping attrs from variables or only dataset level attrs is another question.

+1

What are variable-level attrs generally used for? For semantic information ("collected on 2023-09-30") or encoding-like information ("encoded as unicode")?

[max-sixty]

But should probably have the same arguments and workings as drop_vars and others.

Yes, I was thinking about it a bit more like .reset_encoding — #8259. We could definitely allow passing attrs to drop. I'm not sure what we should do with the errors term, given attrs can be in multiple places...

[max-sixty]

Hi team — what do we think about this? @pydata/xarray

[crusaderky]

To my understanding this should wipe the attrs of the index_vars of the original object?

xarray/xarray/core/dataset.py

Lines 1361 to 1365 in e8be4bb

	for k, v in self._variables.items():
	if k in index_vars:
	variables[k] = index_vars[k]
	else:
	variables[k] = v._copy(deep=deep, data=data.get(k), memo=memo)

[max-sixty]

Atm an Index can't have attrs, at least on main testing ds.indexes['y'].attrs fails.

I added an explicit test for the coords. Lmk if you have anything else we could do for the index...

[benbovy]

I wonder if the invariant checks will pass when testing this on a Dataset with a multi-index.

It might be problematic to copy coordinates variables individually when they have a common index and especially when they all wrap exactly the same index object in their ._data attribute (like in the case of a PandasMultiIndex). A safer approach would be to handle the indexed variables with something like this:

new_variables = {}

for idx, idx_vars in self.xindexes.group_by_index():
    # copy each coordinate variable of an index and drop their attrs
    temp_variables = {k: v.copy() for k, v in idx_vars.items()}
    for v in temp_variables.values():
        v.attrs = {}
    # maybe re-wrap the index object in new coordinate variables
    new_variables.update(idx.create_variables(temp_variables))

[max-sixty]

Thanks, done. I don't think I've done it elegantly, but I'm a bit out of my depth and so won't try and solve perfectly on this pass...

[max-sixty]

Thanks for the review @crusaderky .

I realize it's much easier to review when there are tests; I added those

[shoyer]

My main concern is the meaning of "drop attrs" could be confusing:

• does it refer to dropping specific attrs (like the other drop methods) or all attrs? Potentially this could be controlled by an optional argument. I don't think ds.drop_attrs() would be confusing.
• does it refer to attrs on the top level object or also attrs on coordinates/data variables? The former is what I would expect but some users might want the later, too.

Overall I think this is probably a good idea, though. Certainly I have written this sort of thing many times!

[max-sixty]

My main concern is the meaning of "drop attrs" could be confusing:

I'm very open-minded here! My main motivation is to put less weight on keep_attrs parameter throughout the API.

What's a good way of deciding? We could do what keep_attrs does? Or add a deep param?

[headtr1ck]

Add these methods to api

[max-sixty]

I've now added a deep kwarg, which controls whether it clears the attrs of all variables.

I've provisionally set it as deep=True. This is more aggressive than keep_attrs. What do we think?

[max-sixty]

@pydata/xarray I realize this is still hanging!

What's the best way of resolving? My guess is that there's consensus that the method would be useful, but maybe not on how "deep" it should go. Should we pick something and merge?

[headtr1ck]

Looks good to me.
When you make deep kwarg only, it's easier to add additional arguments later (see other comments)

[max-sixty]

Thanks for the ping @headtr1ck !

Any thoughts from others before we merge?

[dcherian]

let's try it!

(i didn't look at the tests)