Fix internal usage of deprecated APIs triggering user warnings#354
Fix internal usage of deprecated APIs triggering user warnings#354Littie28 wants to merge 4 commits into
Conversation
Added _derivatives and _error_components for internal use. Public APIs retain deprecation warnings as intended. Fixes lmfit#352
There was a problem hiding this comment.
@Littie28 thank you very much for this contribution!
Please make sure you've changed all internal usages of .derivatives and .error_components() to use the private versions. Our CI won't catch cases that you've missed under it's normal configuration at the moment.
| @property | ||
| def derivatives(self): | ||
| """ | ||
| Public wrapper for _derivatives. |
There was a problem hiding this comment.
Please keep the historic docstring on the public, not private derivatives property.
There was a problem hiding this comment.
Moved historic docstring to public api.
| This method assumes that the derivatives contained in the | ||
| object take scalar values (and are not a tuple, like what | ||
| math.frexp() returns, for instance). | ||
| Publich wrapper of _error_compents with FutureWarning. |
There was a problem hiding this comment.
Please keep the historic docstring on the public, not private, error_components() method.
There was a problem hiding this comment.
Moved historic docstring to public api.
|
I searched through the codebase for usages of |
Thanks for that! Let’s use the private versions in the tests. I want to change tests to error on warnings but I don’t want error_components usage to cause tests to fail. In the future I’m interested to move away from derivatives and error components in tests. This will be a reminder for that. |
| from __future__ import division # Many analytical derivatives depend on this | ||
|
|
||
| from builtins import str, zip, range, object | ||
| import collections |
There was a problem hiding this comment.
Please don't reorganize imports. You might have some local automation that is doing that but we don't want these kinds of changes to pollute this PR. I don't disagree with doing this sort of thing, but this PR is not the place.
There was a problem hiding this comment.
All formatting changes are removed
| @property | ||
| def _derivatives(self): | ||
| """ | ||
| Private wrapper for _derivatives without raising a FutureWarning. |
There was a problem hiding this comment.
"""
Private version of `derivatives` for internal use.
"""
The FutureWarning thing is kind of a temporary fleeting issue. Not worth citing in the docstring. Sorry for being picky about wording.
|
|
||
| def _error_components(self): | ||
| """ | ||
| Private wrapper for _error_compents without FutureWarning. |
There was a problem hiding this comment.
"""
Private version of `error_components` for internal use.
"""
The FutureWarning thing is kind of a temporary fleeting issue. Not worth citing in the docstring. Sorry for being picky about wording.
| return (value - self._nominal_value) / self.std_dev | ||
| except ZeroDivisionError: | ||
| raise ValueError("The standard deviation is zero:" " undefined result") | ||
| raise ValueError("The standard deviation is zero: undefined result") |
There was a problem hiding this comment.
Again, this change is ok but this is not the right place to address it. It pollutes the PR.
There was a problem hiding this comment.
All formatting changes are removed
- updated changelog according to feedback - moved historic docstring to public version of derivatives and error_components
- changed all `derivatives` and `error_components` references to private versions `_derivatives` and `_error_components` - reduces warnings raised during test execution from 809 to 352
|
Rebasing to remove the style/formatting changes. |
f7bdd8a to
ca8bf35
Compare
- updated docstrings according to PR discussion
|
Stepping away from |
|
Thanks for the guidance throughout! No rush at all. I'll keep an eye on the PR and address any feedback when you're back. Enjoy the break! |
Added _derivatives and _error_components for internal use. Public APIs retain deprecation warnings as intended.
Fixes #352
pre-commit run --all-fileswith no errors