Update adjust time

[malininae]

This is update of the extract_time preprocessor to make the start_year and end_year optional (aka None). For reasons, see #2304.

Since extract_time is a very common preprocessor, I didn't want to tinker with the order of the input parameters not to make it backwards incompatible. Currently, if the start_year and end_year need to be None, one needs to explicitly specify it into the preprocessor like this:

    extract_time:
        start_year: null
        start_month: 3
        start_day: 1
        end_year: null
        end_month: 7
        end_day: 14

However, another possibility I see, how one can make the default year input as None, to make the default start_month=1, start_day=1, end_month=12, end_day=31. I will leave it to the others to decide. Based on the speed comment from @bouweandela, I kept the original code as much as possible.

[codecov]

Codecov Report

Attention: Patch coverage is 97.14286% with 4 lines in your changes missing coverage. Please review.

Project coverage is 94.77%. Comparing base (21ce35a) to head (20ffb97).

Files with missing lines	Patch %	Lines
esmvalcore/preprocessor/_time.py	97.14%	4 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2490   +/-   ##
=======================================
  Coverage   94.77%   94.77%           
=======================================
  Files         249      249           
  Lines       14081    14087    +6     
=======================================
+ Hits        13345    13351    +6     
  Misses        736      736           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[valeriupredoi]

Many thanks, Liza! A few remarks from me :)

[malininae]

@valeriupredoi the codacy checks fail not on my code, but I guess it popped up because I opened it in VSCode which possibly changed formatting. Should I correct it? Or someone else will do it?

[valeriupredoi]

looking good! Many thanks, Liza! Could you also add a section in the documentaion about this please? It's a nifty feature that'd be nice to have a few words about it in the documentation, as well as the API docs 🍺

[malininae]

looking good! Many thanks, Liza! Could you also add a section in the documentaion about this please? It's a nifty feature that'd be nice to have a few words about it in the documentation, as well as the API docs 🍺

Absolutely, where do you think it would be best to be added? I thought it would come through API only, but I'm happy to elaborate, just not sure where :-)

[valeriupredoi]

looking good! Many thanks, Liza! Could you also add a section in the documentaion about this please? It's a nifty feature that'd be nice to have a few words about it in the documentation, as well as the API docs 🍺

Absolutely, where do you think it would be best to be added? I thought it would come through API only, but I'm happy to elaborate, just not sure where :-)

Liza, there are a few place where a note about this can be nested in:

• doc/recipe/overview.rst in `Recipe section: ``datasets```
• doc/quickstart/find_data.rst in the ICON section - that is a bit special for ICON data finding, not 100% sure this here will work well with it, that's why I'll ask @schlunma for a review here too

[valeriupredoi]

@schlunma it'd be good if you had a second look at this when Liza is all done adding a wee bit of documentation too - it's not 100% clear to me if this will play well with ICON data time gating 🍺

[schlunma]

@malininae would it be possible to revert the changes made by your formatter? This makes it really hard to review 😬

At some point we might apply a formatter to the entire code base, so people could simply use them without the risk of reformatting non-related parts of the code. See also ESMValGroup/ESMValTool#2161.

Thanks!!

[bouweandela]

looking good! Many thanks, Liza! Could you also add a section in the documentaion about this please? It's a nifty feature that'd be nice to have a few words about it in the documentation, as well as the API docs 🍺

Absolutely, where do you think it would be best to be added? I thought it would come through API only, but I'm happy to elaborate, just not sure where :-)

The documentation that needs to be updated is here: https://docs.esmvaltool.org/projects/ESMValCore/en/latest/recipe/preprocessor.html#extract-time as also described in our contribution guidelines here: https://docs.esmvaltool.org/projects/ESMValCore/en/latest/contributing.html#what-should-be-documented

[malininae]

looking good! Many thanks, Liza! Could you also add a section in the documentaion about this please? It's a nifty feature that'd be nice to have a few words about it in the documentation, as well as the API docs 🍺

Absolutely, where do you think it would be best to be added? I thought it would come through API only, but I'm happy to elaborate, just not sure where :-)

The documentation that needs to be updated is here: https://docs.esmvaltool.org/projects/ESMValCore/en/latest/recipe/preprocessor.html#extract-time as also described in our contribution guidelines here: https://docs.esmvaltool.org/projects/ESMValCore/en/latest/contributing.html#what-should-be-documented

Thanks @bouweandela. As far as I understood, sphinx grabs the documentation for the preprocessor functions from the docstrings. I looked at what was built and the updated parameter texts are there. @valeriupredoi I looked and are you sure there is a sense to add it to datasets? It's just in a preprocessor function, not at the timerange part. At the same time, happy to add if you think it's necessary.

[malininae]

@schlunma @valeriupredoi the formatting issue seemed to be resolved by updating the branch.

[schlunma]

No, unfortunately the changes are still there. I think you only actively changed the extract_time code, but when you look in the "Files changed" tab from this PR, you'll see that there are lots of changes not related to that.

Merging main into this won't fix it, you'll need to adapt it manually (either by reverting line by line or by restoring the original file from main and then re-adding you're desired code changes). Please make sure to not run the code formatter (yapf?) after making these changes.

[bouweandela]

Thanks @bouweandela. As far as I understood, sphinx grabs the documentation for the preprocessor functions from the docstrings. I looked at what was built and the updated parameter texts are there.

The docstrings are used to build the API docs page here, but you'll also need to update the page that describes how to use this preprocessor function from a recipe:

ESMValCore/doc/recipe/preprocessor.rst

Lines 1307 to 1325 in 21ce35a

	``extract_time``
	----------------
	This function subsets a dataset between two points in times. It removes all
	times in the dataset before the first time and after the last time point.
	The required arguments are relatively self explanatory:
	* ``start_year``
	* ``start_month``
	* ``start_day``
	* ``end_year``
	* ``end_month``
	* ``end_day``
	These start and end points are set using the datasets native calendar.
	All six arguments should be given as integers - the named month string
	will not be accepted.
	See also :func:`esmvalcore.preprocessor.extract_time`.

/ https://docs.esmvaltool.org/projects/ESMValCore/en/latest/recipe/preprocessor.html#extract-time

[bouweandela]

Please make sure to not run the code formatter (yapf?) after making these changes.

It looks like the pre-commit hooks are not playing nice in this case. Maybe we should discuss this again in the @ESMValGroup/technical-lead-development-team team to see how we can make this more smooth. I usually stage my files for committing by e.g. running git add -u and then run git diff --staged | yapf-diff -i to only format the diff, or I do run the pre-commit hooks and then select only some changes for committing by running git add -p. That works, but it's clunky and not what new contributors expect. Just being able to use pre-commit and be done with it would be much nicer. See also ESMValGroup/ESMValTool#2161.

@schlunma Until we have fixed our automated formatting and pre-commit hooks, I would suggest that reviewers of pull requests are lenient about formatting changes and take a bit of extra trouble to review those too. Would that work for you?

[malininae]

Please make sure to not run the code formatter (yapf?) after making these changes.

It looks like the pre-commit hooks are not playing nice in this case. Maybe we should discuss this again in the @ESMValGroup/technical-lead-development-team team to see how we can make this more smooth. I usually stage my files for committing by e.g. running git add -u and then run git diff --staged | yapf-diff -i to only format the diff, or I do run the pre-commit hooks and then select only some changes for committing by running git add -p. That works, but it's clunky and not what new contributors expect. Just being able to use pre-commit and be done with it would be much nicer. See also ESMValGroup/ESMValTool#2161.

@schlunma Until we have fixed our automated formatting and pre-commit hooks, I would suggest that reviewers of pull requests are lenient about formatting changes and take a bit of extra trouble to review those too. Would that work for you?

Thanks @bouweandela and @schlunma, do I understand correctly, that you want me to revert to the old red changes even though the checks passed? I was thinking, I did ran yapf back in the day, could that be it who screwed stuff up? Is there a way to make the code (I guess pylint compatible?) with running some sort of formatter? I tried ruff (didn't suggest any changes) and black, who made things much worse even with options of shorter lines and py310 as a target.