Fixed FutureWarning in Series.pct_change by specifying fill_method=None to avoid deprecated default behavior. #54
Conversation
…ne to avoid deprecated default behavior.
|
My review is in progress 📖 - I will have feedback for you in a few minutes! |
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
There was a problem hiding this comment.
I have reviewed your code and found 2 potential issues. To discuss my individual comments that I have added, tag me in replies using @korbit-ai.
Please react with a 👍 to my comments that you find helpful and a 👎 to those you find unhelpful - this will help me learn and improve as we collaborate.
| @@ -208,9 +210,9 @@ def _prepare_returns(data, rf=0.0, nperiods=None): | |||
| if isinstance(data, _pd.DataFrame): | |||
| for col in data.columns: | |||
| if data[col].dropna().min() >= 0 and data[col].dropna().max() > 1: | |||
| data[col] = data[col].pct_change() | |||
| data[col] = data[col].pct_change(fill_method=None) | |||
There was a problem hiding this comment.
The change to use pct_change(fill_method=None) addresses the FutureWarning, but we should consider if this is the optimal approach for all scenarios. In some cases, we might want to fill NaN values differently. Consider adding a parameter to the functions that use pct_change() to allow customization of the fill_method when necessary. This would provide more flexibility while still addressing the deprecation warning.
| @@ -208,9 +210,9 @@ def _prepare_returns(data, rf=0.0, nperiods=None): | |||
| if isinstance(data, _pd.DataFrame): | |||
| for col in data.columns: | |||
| if data[col].dropna().min() >= 0 and data[col].dropna().max() > 1: | |||
| data[col] = data[col].pct_change() | |||
| data[col] = data[col].pct_change(fill_method=None) | |||
There was a problem hiding this comment.
The changes in this pull request involve the use of the 'fill_method' parameter in the 'pct_change()' function. It's important to ensure that the function behaves as expected when different arguments are passed to this parameter. I recommend adding parameterized tests to verify this. These tests should cover all possible values that could be passed to the 'fill_method' parameter.
|
For both issues, considering the apllication (_prepare_returns function), and considering the code following it modifcations handle missing values adequately, I believe the 'fill_method=None' is appropriate. It wouldn't make sense to pad (or forward-fill) percentage changes. |
Resolved FutureWarning in Series.pct_change in
quantstats_lumi/utils.py,quantstats_lumi/_plotting/core.pyandquantstats_lumi/_plotting/wrappers.pyby specifying fill_method=None. The default behaviour (fill_method='pad' frompandas.pct_change()) is deprecated and will be removed in a future version, which could cause issues with NA values. By setting fill_method=None, we ensure that NA values are not filled, avoiding potential errors.Description by Korbit AI
Note
This feature is in early access. You can enable or disable it in the Korbit Console.
What change is being made?
Specify
fill_method=NoneinSeries.pct_changecalls to avoid deprecated default behavior and fix FutureWarning.Why are these changes being made?
The default behavior of
Series.pct_changeis deprecated, and specifyingfill_method=Noneensures future compatibility and removes the FutureWarning. This change improves code robustness and clarity by explicitly defining the fill method.