Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate mf_flag call signature in xclim.ensembles, expose calendar conversion utilities, fix _gen_returns_section bug #1317

Merged
merged 14 commits into from
Mar 10, 2023

Conversation

Zeitsperre
Copy link
Collaborator

@Zeitsperre Zeitsperre commented Mar 9, 2023

Pull Request Checklist:

  • This PR addresses an already opened issue (for bug fixes / features)
  • Tests for the changes have been added (for bug fixes / features)
    • (If applicable) Documentation has been added / updated (for bug fixes / features)
  • CHANGES.rst has been updated (with summary of main changes)
    • Link to issue (:issue:number) and pull request (:pull:number) has been added

What kind of change does this PR introduce?

  • Changes the call signature for xclim.ensembles.create_ensemble() and _ens_align_datasets() to use multifile in lieu of mf_flag.
  • Updates docstrings and emits warnings for deprecated usage on those call signatures.
  • Exposes two previously private functions that can return day of year indexes explicitly or randomly.
  • Moves the module docstring from xclim.indices into the indices documentation file.
  • Fixes a formatting bug in _gen_returns_section that caused poor rendering in the documentation

Does this PR introduce a breaking change?

No*. Original calls with kwargs mf_flag set will still work, but will emit warnings that users should update their scripts. This will be a major breaking change in xclim v0.43.0. The new functions were previously inaccessible (not breaking).

Other information:

@Zeitsperre Zeitsperre added standards / conventions Suggestions on ways forward API Interfacing and User Concerns labels Mar 9, 2023
@Zeitsperre Zeitsperre added this to the v0.42 milestone Mar 9, 2023
@Zeitsperre Zeitsperre requested review from bzah and aulemahal March 9, 2023 21:25
@Zeitsperre Zeitsperre self-assigned this Mar 9, 2023
@Zeitsperre Zeitsperre mentioned this pull request Mar 9, 2023
12 tasks
@Zeitsperre
Copy link
Collaborator Author

Question: Should I deal with the other API-related comment in this PR?

Other example is xclim.core.convert_calendar. It has 2 private functions (_yearly_interp_doy, _yearly_random_doy) that could be out of the main function. This makes the if statements easier to follow due to the code has a very large if statement.

Copy link
Contributor

@bzah bzah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!
There are not too many changes here, so from a reviewer pov I would not mind if you also included the other API changes in the same PR.

@Zeitsperre Zeitsperre changed the title Deprecate mf_flag call signature in xclim.ensembles Deprecate mf_flag call signature in xclim.ensembles, expose calendar conversion utilities Mar 10, 2023
@Zeitsperre
Copy link
Collaborator Author

This PR is good for review!

@github-actions github-actions bot added docs Improvements to documenation indicators Climate indices and indicators labels Mar 10, 2023
Copy link
Collaborator

@aulemahal aulemahal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed!
We had the same Future vs Deprecation issue in xscen this morning...

xclim/ensembles/_base.py Outdated Show resolved Hide resolved
xclim/ensembles/_base.py Outdated Show resolved Hide resolved
xclim/testing/tests/test_ensembles.py Outdated Show resolved Hide resolved
@github-actions github-actions bot added the approved Approved for additional tests label Mar 10, 2023
@Zeitsperre Zeitsperre force-pushed the api-breaking-changes branch from c564fcc to 79c82de Compare March 10, 2023 20:06
@Zeitsperre
Copy link
Collaborator Author

Returns formatting for Indicators is improved:
image

@aulemahal
Copy link
Collaborator

Way better than before, thanks!
Ze best would be to have a second bullet point level for the "description" attribute I think, really struggled to find how to do this when I implemented this part. But this is already much more readable.

@Zeitsperre Zeitsperre changed the title Deprecate mf_flag call signature in xclim.ensembles, expose calendar conversion utilities Deprecate mf_flag call signature in xclim.ensembles, expose calendar conversion utilities, fix _gen_returns_section bug Mar 10, 2023
@Zeitsperre Zeitsperre merged commit 5af0d57 into master Mar 10, 2023
@Zeitsperre Zeitsperre deleted the api-breaking-changes branch March 10, 2023 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Interfacing and User Concerns approved Approved for additional tests docs Improvements to documenation indicators Climate indices and indicators standards / conventions Suggestions on ways forward
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[PyOpenSci] Reviewer #1 Comments
3 participants