add custom season method

[jukent]

PR Summary

Adds custom season list as kwarg for climatology_average
Closes #198

PR Checklist

General

• Make an issue if one doesn't already exist
• Link the issue this PR resolves by adding closes #XXX to the PR description where XXX is the number of the issue.
• Add a brief summary of changes to docs/release-notes.rst
• Add appropriate labels to this PR
• Make your changes in a forked repository rather than directly in this repo
• Open this PR as a draft if it is not ready for review
• Convert this PR from a draft to a full PR before requesting reviewers
• Request @NCAR/geocat for reviews
• Passes precommit. To set up on your local, run pre-commit install from the top level of the repository. To manually run pre-commits, use pre-commit run --all-files and re-add any changed files before committing again and pushing.

Functionality

• Function is in appropriate module file
• New function(s) intended for public API added to src/geocat/comp/__init__.py file

Testing

• Tests for function exists in associated module test file
• Tests cover all possible logical paths in your function

Documentation

• Docstrings have been added to all new functions (Documentation Standards)
• Docstrings have updated with any function changes
• Internal functions have a preceding underscore (_) and have been added to docs/internal_api/index.rst
• User facing functions have been added to docs/user_api/index.rst under their module

[jukent]

Target final dataset:

vs what we currently have:

I have the values matching up, and am still wrestling with the Xarray dimension(?) name where it says SON instead of season. (it says dimensions are (`SON1,) but when I go to rename the dimension it says SON isn't a dimension so idk)

Also I'm not sure if we want to add a pandas season index/ if that means something specific to the typical seasons (will research this).

And one issue is that in order to get this values. I have to comment out this block of code borrowed from month_to_season:

# For this season, the last "mean" will be the value for Dec so we drop the last month
if season == 'DJF':
    data_filter = data_filter.isel(
        {time_dim: slice(None, -1)})
# For this season, the first "mean" will be the value for Jan so we drop the first month
elif season == 'NDJ':
    data_filter = data_filter.isel(
        {time_dim: slice(1, None)})

or else that values are off in DJF. I need to look at this math more and know if this is right to not implement (as in are we doing it correctly in the non-custom season version). Any thoughts on that @anissa111

I want to answer these questions and solve the dimension thing before I add tests, so I know whether to say identical objects or identical values and dimensions.

[jukent]

Didn't expect this to fail tests in test_climate_anomaly as well as test_climate_average. Will look into this tomorrow hopefully.

[anissa111]

@jukent I'm not sure I know enough about that to have any valuable thoughts right now. I think it could make sense that the two implementations are different enough that this version wouldn't need that block, but I'd want to understand the underlying reason why, does that make sense?

[jukent]

Yeah that makes sense. I think when we want winter from one year, we have to deal with it being over the year break. But when we want winter from all years, it doesn't matter if January is in this winter or the next. Like the grouping of December 22 of January 23 together.

There is still the very first and very last year that could be affected? And that causes the small discrepancy. My hunch is that since we're going through months first it isn't needed or wanted but I'll lay it out better tomorrow.

[jukent]

Related to #313

[anissa111]

I'm a bit concerned with the logic flow as written as mentioned in a few of my comments below for a few reasons, some of which predate this PR

1. I am a little uncozy with the logic structure of

if <this setting>:
    do all analysis
    return

if <next setting>:
    do all analysis
    return

2. This introduces some edge cases such as what happens if the user specifies custom seasons, but a frequency of anything other than 'season'
3. Apart from the general uncozy mentioned in (1), I think we could be making better use of the custom_seasons optional argument here. What if we used a default of custom_seasons = ['DJF', 'MAM', 'JJA', 'SON'] and then performed the calculation for seasons in the same way instead of separating the season calculation into a "custom" or "default" section? I would think, ideally, the same calculations for custom season should also work for the standard, correct?
4. Seeing the code in this PR, I'm again not totally sure why you've changed some of the inputs to kwargs instead of just arguments.

Anyway, sorry for the late review, let me know if you'd like a dedicated time for me to explain some of my questions/thinking on suggestions!

[jukent]

2. If the user specifies custom seasons but daily climatoloty, currently the frequency trumps it and the user would get daily climatology. You're right to point that out though. I think that is the behavior we want, but maybe raise a warning to say that their custom seasons will be ignored.
3. Good points. We could do some analysis to see if Xarray's method is faster at all -- but that would work and sounds cleaner for sure.

I'll address your other points and make some edits on Monday.

[jukent]

Passing local tests!

[anissa111]

Mostly just comments from this morning's meeting. I did manage to write something that might be a little more readable for that one section I said I'd take a stab at.

[anissa111]

This should be good to go when the tests finish! Good job!!