Support monthly and weekly data for preprocess module

[geek-yang]

Support monthly/weekly data in preprocess by providing a timescale argument to the user.

This closes #168 and closes #160 .

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[geek-yang]

For the unittest below, it fails only for python 3.8. It seems a version conflict between pandas and xarray causes the failure of resampling:

s2spy/tests/test_preprocess.py

Lines 83 to 88 in 5abf569

	@pytest.mark.skipif(
	sys.version_info < (3, 9),
	reason="Resample fails due to version conflict of xarray and pandas.",
	)
	def test_get_climatology_monthly(self, raw_field):
	raw_field_monthly = raw_field.resample(time="M").mean()

But since it only relates to the preparation of testing data, not about the code. I simply silent it for python 3.8. Just let me know if you have better solutions. Thanks @BSchilperoort !

[BSchilperoort]

Hi Yang, I still have to test the code, but here are already some comments :)

[BSchilperoort]

I am not so sure about having this alias. Users will see this in the documentation, and when accessing the class/function using help(...):

Perhaps this works better if you define Timescale as an alias (see the python docs).

from typing import TypeAlias
Timescale: TypeAlias = Literal["monthly", "weekly", "daily"]

You can try that, and see if it shows up in the documentation or not.

[geek-yang]

Thanks for raising this point! It seems using TypeAlias is more formal.

However, just find that it also couldn't be spelled out in the documentation.

Both cases work well with help(...)

[geek-yang]

Oh no....TypeAlias is available from python 3.10. In python <= 3.9 it should be imported from typing_extensions. (https://mypy.readthedocs.io/en/stable/kinds_of_types.html#type-aliases)

[BSchilperoort]

https://www.sphinx-doc.org/en/master/usage/extensions/autodoc.html#confval-autodoc_type_aliases it seems like sphinx autodoc does resolve the aliases (unless you explicitly don't want that).

sphinx-autoapi (what we use) doesn't seem to support this.

[semvijverberg]

This is starting to look elegant :) 👀

Showing the options (Literal['monthly', 'weekly', 'daily']) would be nice in the docs. If I understand correctly, our sphinx-autodoc does not resolve this (yet), and shinx-autoapi does?

Is it an option to switch? or is there a downside?

[geek-yang]

It is too much to switch from autoapi to autodoc simply for an unresolved alias 😂 , given that they are a bit different and autoapi works quite well for now.

I have added a hint in the tutorial notebook tutorial_preprocessing.ipynb:
"User needs to inform preprocessor the temporal resoltuion of their data, by setting the timescale argument to be either daily, weekly or monthly timescale."

And we have very clear error message to the user about the supported timescales:

s2spy/s2spy/preprocess.py

Lines 131 to 138 in f157ab2

	def _check_temporal_resolution(timescale: Timescale) -> Timescale:
	support_temporal_resolution = ["monthly", "weekly", "daily"]
	if timescale not in support_temporal_resolution:
	raise ValueError(
	"Given temporal resoltuion is not supported."
	"Please choose from 'monthly', 'weekly', 'daily'."
	)
	return timescale

I think this should be sufficient.

[semvijverberg]

I can absolutely live with that 😆 . Thanks a lot for the work @geek-yang and the checking @BSchilperoort!

[BSchilperoort]

But since it only relates to the preparation of testing data, not about the code. I simply silent it for python 3.8

It's not ideal, but I also wouldn't want to spend too much time on that. Perhaps we have to deprecate python 3.8 anyway: xarray isn't supporting it anymore for new releases.

[geek-yang]

But since it only relates to the preparation of testing data, not about the code. I simply silent it for python 3.8

It's not ideal, but I also wouldn't want to spend too much time on that. Perhaps we have to deprecate python 3.8 anyway: xarray isn't supporting it anymore for new releases.

I agree. Let's drop python 3.8.

[semvijverberg]

Hi Yang,

I pulled the branch to test it :)

In my test, temporal_resolution for monthly data is 31. According to the statement, max_days (31) is not larger than 31 ;)
if not max_days > temporal_resolution > min_days:

Should be:
if not max_days >= temporal_resolution >= min_days:

[geek-yang]

Oh! Yes, of course! Thanks for fixing this!

[BSchilperoort]

Just the annoying lack of type alias support in sphinx autoapi :/

Otherwise everything looks good.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

91.8% Coverage
0.0% Duplication

[semvijverberg]

Thanks @geek-yang @BSchilperoort!