Update spatial axis arg supported type and keys

[tomvothecoder]

Description

• Closes [FEATURE]: Update supported spatial axis arg keys generic format #225

Summary of changes

• Update "lat" and "lon" references to "Y" and "X"
  • I decided to not allow "y" and "x" because they aren't CF-compliant (even if we could do .upper() to convert them).
  • https://github.com/XCDAT/xcdat/blob/79c488ea890febc422fe2e38b85c4e9dc7c72565/xcdat/axis.py#L9-L25
• Remove _get_generic_axis_keys() since it is no longer needed
• Add examples to SpatialAccessor.average() docstring
• Update TemporalAccessor.__init__ check for 'T' axis
• Add TemporalAccessor.average() docstring examples

Checklist

• My code follows the style guidelines of this project
• I have performed a self-review of my own code
• My changes generate no new warnings
• Any dependent changes have been merged and published in downstream modules

If applicable:

• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass with my changes (locally and CI/CD build)
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• I have noted that this is a breaking change for a major release (fix or feature that would cause existing functionality to not work as expected)

[codecov-commenter]

Codecov Report

Merging #226 (8e9e61c) into main (1fdc8a9) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #226   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            9         8    -1     
  Lines          742       691   -51     
=========================================
- Hits           742       691   -51     

Impacted Files	Coverage Δ
xcdat/__init__.py	100.00% <ø> (ø)
xcdat/utils.py	100.00% <ø> (ø)
xcdat/bounds.py	100.00% <100.00%> (ø)
xcdat/dataset.py	100.00% <100.00%> (ø)
xcdat/spatial.py	100.00% <100.00%> (ø)
xcdat/temporal.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 4e582e5...8e9e61c. Read the comment docs.

[tomvothecoder]

Related to #166 (comment)

[pochedls]

Should this be "Validation that the axis dimension exists..."?

[pochedls]

I like how you wrote this below: "List of axis dimensions or a single axis dimension"

[tomvothecoder]

I meant to use the bottom description, thanks for the catch

[pochedls]

In CDAT, you specify the axes as a single string, e.g., 'xy'. I think it would be worth including that as a method for specifying the axes. I think this could be done simply by tweaking the _validate_axis_arg. Something like this (which could be combined with this line):

if isinstance(axis, str):
    # convert string axis argument to list of strings
    # representing the axes
    axis = [adim.upper() for adim in axis]

What do you think? Other than that potential change this looks great!

[tomvothecoder]

Thanks for the review!

In CDAT, you specify the axes as a single string, e.g., 'xy'. I think it would be worth including that as a method for specifying the axes.

It is more Pythonic (and probably standard coding practice) for a function argument to accept a list of strings (axis=["X", "Y", "Z", "T"]) over a single string (axis="xyzt"), and to limit the accepted argument types as much as possible.

In this case, each string should be treated as an independent instance, rather than coupled as a single instance that has to be parsed. There is also less logic involved with accepting just a list of strings because type checking isn't necessary, the behavior of the API is more predictable, and it is more explicit and readable (in my opinion).

Posts I found on this subject:

---

https://stackoverflow.com/a/51737249

Parameters that can be either a thing or an iterable or things are a code smell. It’s even worse when the thing is a string, because a string is an iterable, and even a sequence (so your test for isinstance(names, Iterable) would do the wrong thing).

This is what we're currently doing for a single axis (axis="X").

But if the best design really is a string or a (non-string) iterable of strings, or a string or a tuple of strings, the standard way to do that is type switching. It may look a bit ugly, but it calls attention to the fact that you’re doing exactly what you’re doing, and it does it in the most idiomatic way.

def spam(names):
if isinstance(names, str):
names = [names]
dostuff(names)

I do agree with this person that it is ugly to have to perform this check for every function that has an argument that accepts a string or a list of strings.

https://stackoverflow.com/questions/998938/handle-either-a-list-or-single-integer-as-an-argument/998949#998949

expect that the argument will always be a list - even if its just a list of one element.

Remember:

It is easier to ask for forgiveness than permission.

---

I suggest that we accept just a list of strings unless there is a strong reason otherwise.

Examples of valid axis arg values:

• ["X", "Y", "T"]
• ["X"]

Examples of invalid axis arg values:

• "xyt"
• "XYT"
• "X"
• "x"

[pochedls]

Thanks for the detailed reply.