-
Notifications
You must be signed in to change notification settings - Fork 28
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
Use scipy.stats.median_abs_deviation in outliers.hampel #81
Conversation
scipy.stats.median_absolute_deviation is deprecated in version 1.5
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.
Let's think about the scipy bump first
@@ -1,5 +1,5 @@ | |||
numpy>=1.16.0 | |||
pandas>=0.25.0,<1.1.0 | |||
pvlib>=0.8.0 | |||
scipy>=1.3.0 | |||
scipy>=1.5.0 |
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.
Maybe think about this for a moment - pvlib is on scipy >= 1.2.0, and it's a guiding principle, but not a hard rule, that pvanalytics and pvlib play nice together. Do we want to bump the scipy version? Or, program around the deprecation warning?
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.
PVLib tests pass at scipy 1.5.2. I'm personally okay with the different requirement, so long as it satisfies the >= cirteria. We already depend on scipy > 1.2 for the median_absolute_deviation
function.
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.
As someone that recently started trying to use pvanalytics alongside other code, I have two comments:
- this is a big jump to a newish scipy version (4 months), and would make me think twice about installing pvanalytics in some places and prevent it in others.
- it would help to know the pvanalytics policy for dependencies. My suggestion is following NEP-29 for the core pydata packages. I'm not sure what to recommend for a pvlib version policy.
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.
Thanks @wholmgren. I was not aware of NEP-29, and I certainly see the benefits for a predictable approach to dependency management. My approach has been ad-hoc thus far, so I would not be opposed to adopting the policy laid out in NEP-29: all minor versions back 2 years.
That will mean a bit of re-working since some versions have crept higher than that would allow. For instance, to support scipy==1.2 we will need to add statsmodels
as a dependency and use statsmodels.robust.scale.mad()
here.
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.
Ok to add statsmodels. As far as the median absolute deviation statistic, an imprecise value is good enough here.
Closing in favor of #85. |
scipy.stats.median_absolute_deviation is deprecated in version 1.5
Checklist