Rework mappers to be more general

[malmans2]

• closes better mappers draft #175, closes Preferred way to deal with indexed coordinates? #107
• Add more tests
• Copy over Use standard_name mapper in rename_like #165 tests
• Explicitly add conflicting functions

@dcherian were you thinking something along these lines?

Currently _get_dims/coords/indexes use _get_axis_coord and _get_with_standard_name.
We can probably get rid of _get_axis_coord and _get_with_standard_name, I just wanted to check first whether that's the way to go as they are used in many places. Not sure if all _get_axis_coord can be replaced by the same function (e.g., _get_coords) or it's not that simple.

All tests are passing, I didn't add new tests yet.

[dcherian]

yes! something very similar see #175.

I would use apply_mapper though to apply them. and I think _get_single should be a decorator. So in the dictionary we would have something like "indexes": (_single(_get_indexes))

Can you unify the two PRs?

[codecov-io]

Codecov Report

Merging #174 (bc2b14d) into main (ed3dcc1) will increase coverage by 0.05%.
The diff coverage is 96.00%.

@@            Coverage Diff             @@
##             main     #174      +/-   ##
==========================================
+ Coverage   95.86%   95.91%   +0.05%     
==========================================
  Files          11       11              
  Lines        1305     1347      +42     
==========================================
+ Hits         1251     1292      +41     
- Misses         54       55       +1     

Flag	Coverage Δ
unittests	95.91% <96.00%> (+0.05%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
cf_xarray/datasets.py	100.00% <ø> (ø)
cf_xarray/accessor.py	94.57% <93.18%> (-0.03%)	⬇️
cf_xarray/tests/test_accessor.py	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ed3dcc1...bc2b14d. Read the comment docs.

[malmans2]

Should we try to use the new functions as much as possible?
For example, let's use _single(_get_all)(var, key) rather than _get_axis_coord_single(var, key)?

[dcherian]

Yes I think that is right. You'll basically implement #165 too :) if you change all the calls to get_axis_coord_single. Copy the tests over?

[malmans2]

Sounds good! I started with _get_axis_coord_single always replaced by _single(_get_all) (all current tests are passing, but can you check whether _get_all is always the right one. I think so, but I'm not 100% sure).

[malmans2]

Actually, the readthedocs action caught it. Should be _get_coords for plots.

[malmans2]

I'm not following what's happening here:

cf-xarray/cf_xarray/accessor.py

Lines 813 to 837 in 425c472

	def __call__(self, *args, **kwargs):
	"""
	Allows .plot()
	"""
	plot = _getattr(
	obj=self._obj,
	attr="plot",
	accessor=self.accessor,
	key_mappers=dict.fromkeys(self._keys, (_single(_get_coords),)),
	)
	return self._plot_decorator(plot)(*args, **kwargs)
	def __getattr__(self, attr):
	"""
	Wraps .plot.contour() for example.
	"""
	return _getattr(
	obj=self._obj.plot,
	attr=attr,
	accessor=self.accessor,
	key_mappers=dict.fromkeys(self._keys, (_single(_get_all),)),
	# TODO: "extra_decorator" is more complex than I would like it to be.
	# Not sure if there is a better way though
	extra_decorator=self._plot_decorator,
	)

I must use _get_coords in __call__, otherwise this fails:

from cf_xarray.datasets import airds
airds.air.cf.weighted("area").mean(["latitude", "time"]).cf.plot(x="longitude")

Error is ValueError: Cannot specify both x and y kwargs for line plots

However, _get_all is fine in __getattr__, and this is OK:

airds.air.cf.weighted("area").mean(["latitude", "time"]).cf.plot.line(x="longitude")

[dcherian]

Phew after some debugging...

>>> da = airds.air.cf.weighted("area").mean(["latitude", "time"])
>>> cf_xarray.accessor._get_with_standard_name(da, None)
['<this-array>']

Also

cf_xarray.accessor._get_with_standard_name(airds, None)
["cell_area"]

We were also missing one line plot case (.plot with 1D arrays) which meant we tried to set x and y. since y=None the standard name mapper returned [this-array] somehow.

[malmans2]

Should we merge _get_axis_coord and _get_measure so we only loop once? We still use _get_with_standard_name outside of _get_all:

cf-xarray/cf_xarray/accessor.py

Line 631 in a3178b5

stdnames = set(_get_with_standard_name(obj, k))

[dcherian]

Should we merge _get_axis_coord and _get_measure so we only loop once?

I'd like to keep them separate for now since they do different things. One optimization is to add keys_to_search as a kwarg to the mapper functions. for example,_get_indexes would be _get_all(..., keys_to_search=obj.indexes). Then _get_all would only loop over keys_to_search instead of obj.variables

We can do this later though. This is a nice change already.

[malmans2]

Mmmm, I don't think we are getting the right error here:

from cf_xarray.datasets import popds
popds.cf.differentiate("longitude")

ValueError: Coordinate longitude does not exist.

The error should be raised before by @_single

Same for `popds.cf.drop_dims("longitude")

[malmans2]

Should we somehow always set this True when using @_single?

cf-xarray/cf_xarray/accessor.py

Lines 176 to 177 in bc2b14d

	if error:
	raise e

[dcherian]

Yes looks right

[malmans2]

Actually, I think we can still allow the case with no results. Let's use a very specific message here, so we know this one should always be raised, or an error different than KeyError?

cf-xarray/cf_xarray/accessor.py

Lines 409 to 412 in bc2b14d

	if len(results) > 1:
	raise KeyError(
	f"Multiple results for {key!r} found: {results!r}. I expected only one."
	)

[dcherian]

KeyError seems right, what do you want to change? It looks like longitude didn't get mapped to anything. isn't that the issue?

[malmans2]

from cf_xarray.datasets import popds
popds.cf.differentiate("longitude")

I think longitude is mapped to 2 variables, but _rewrite_values switches off the error from single?

cf-xarray/cf_xarray/accessor.py

Lines 975 to 978 in bc2b14d

	newvalue = [
	apply_mapper(mappers, self._obj, v, error=False, default=[v])
	for v in value
	]

Error should be "Multiple results for 'longitude' found: ['ULONG', 'TLONG']. I expected only one."

[dcherian]

I say we raise ValueError in _single and open an issue to come up with a better solution

[malmans2]

Sounds good

[dcherian]

in apply_mapper we could also search for "expected only one" in the exception message and raise in that case even when error is False

[malmans2]

Yes that's my favorite solution but I think we need a slightly more specific error because "expected only one" is also used by check_results (although e is upper case in the latter).

[malmans2]

Should be almost good to go. Unless you think we should add more tests to make sure we cover all signatures and their xarray functions, we can probably merge and explicitly add functions to the accessor later if we find conflicts.

[dcherian]

LGTM. Thanks @malmans2