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
Remove deprecated indicators/indices, handle warnings #1318
Conversation
…cover_duration, continuous_snow_cover_start, continuous_snow_cover_end) and indices (snow_cover_duration, continuous_snow_cover_start, continuous_snow_cover_end), catch existing DeprecationWarnings, migrate deprecated test calls to new functions, note methods and indices that may require internal logic revisions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realized that we are emitting DeprecationWarning
instead of FutureWarning
. As we discovered elsewhere, the former is not visible to the user by default... Which means, that all the deprecation we are removing here have not been properly announced to the users...
Is the HISTORY enough to justify removing them? Or do we want to wait one more version ?
At least, I think we should fix the issue here :
Lines 1351 to 1357 in e0f67df
def _show_deprecation_warning(self): | |
warnings.warn( | |
f"`{self.title}` is deprecated as of xclim v{self._version_deprecated}. " | |
f"See the xclim release notes for more information.", | |
DeprecationWarning, | |
stacklevel=3, | |
) |
I think we would be fine effecting these changes. In some cases, the history has indicated that we are removing these functions as far back as v0.39, however, if we opt to wait until v0.43, I could remove all remaining deprecated indicators/indices as well. If we want to mark this PR for v0.43, I'd open another PR to deal with the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✝️ 🎤 💦
Ite in pace, glorificando vita vestra Dominum
Amen.
Pull Request Checklist:
discharge_distribution_fit
has wrong_registry_id
#1281number
) and pull request (:pull:number
) has been addedWhat kind of change does this PR introduce?
Indices that emit FutureWarnings through regular use have been identified:snowfall_approximation
(method='brown'
)effective_growing_degree_days
convert_units_to
calls #1319Does this PR introduce a breaking change?
Yes. This removes all currently-marked deprecated indices and indicators.
Other information:
The volume of warnings emitted from
pytest
is significantly reduced. The only warnings now emitted are those coming from lower-level libraries, likenumpy
andxarray
. This should make it easier to identify and handle these instances moving forward.