[MNT] rename MeanNormalisationScaler public name to british spelling#968
[MNT] rename MeanNormalisationScaler public name to british spelling#968favourone wants to merge 1 commit into
Conversation
|
The two required CircleCI jobs failed because the California housing dataset download returned HTTP 403 Forbidden. The other 11 checks passed. I do not have permission to rerun the workflow. Could a maintainer please rerun it? |
solegalli
left a comment
solegalli
left a comment
There was a problem hiding this comment.
Hey @favourone
Great job working on this fix. Well done.
Could you explore the possibility of adding a future warning to the old version of the class, so that when a user (or an LLM) use the old class in their projects, they see the warning and revert to the new way of calling it? I added more details in the comments.
I think we should raise this warning, instead of pointing to the backward compatible class in the user guide or docstrings, which we would remove anyways in future releases.
| :class:`MeanNormalisationScaler()` only works with non-constant numerical variables. | ||
| If the variable is constant, the scaler will raise an error. | ||
|
|
||
| ``MeanNormalizationScaler`` remains available as a backward-compatible alias. |
There was a problem hiding this comment.
Could we remove this line from the user docs?
| Feature-engine extends scikit-learn's scaling functionality with the following transformer: | ||
|
|
||
| - :doc:`api_doc/scaling/MeanNormalizationScaler`: scale variables using mean normalisation | ||
| - :doc:`MeanNormalisationScaler <api_doc/scaling/MeanNormalizationScaler>`: scale variables using mean normalisation |
There was a problem hiding this comment.
| - :doc:`MeanNormalisationScaler <api_doc/scaling/MeanNormalizationScaler>`: scale variables using mean normalisation | |
| - :doc:`<api_doc/scaling/MeanNormalizationScaler>`: scale variables using mean normalisation |
|
|
||
| More details in the :ref:`User Guide <mean_normalisation_scaler>`. | ||
|
|
||
| The ``MeanNormalizationScaler`` spelling remains available as a |
There was a problem hiding this comment.
I'd remove this from the documentation. If people have legacy code, they'll realise there's been a change with a warning that should raise the class with the american spelling
|
|
||
|
|
||
| # Backward-compatible alias for the original American spelling. | ||
| MeanNormalizationScaler = MeanNormalisationScaler |
There was a problem hiding this comment.
Is there a way to trigger a warning when the user calls the class with z instead of s? I though that if instead of equal we used inheritance, then we could add the warning in the init or something like that. Could you research this possibility? Something like MeanNormalizationScaler was deprecated in favour of MeanNormalisationScaler in version 2.0.0 and will be removed in version 2.1.0. To silence the warning, use MeanNormalisationScaler instead.
| from tests.estimator_checks.fit_functionality_checks import check_return_empty | ||
|
|
||
|
|
||
| def test_mean_normalization_scaler_is_backward_compatible_alias(): |
There was a problem hiding this comment.
is this the best way to assess that both classes have the same behaviour? Somehow I thought that we could pass the different classes to all tests using parametrize. We should also test that using the old class raises the future warning.
I am aware of this issue. I need to fix it :) |
solegalli
changed the title
2 participants
Summary
MeanNormalisationScaleras the preferred public class name.MeanNormalizationScaleras a backward-compatible alias.feature_engine.scaling.Tests
python -m pytest tests/test_scaling/test_mean_normalization.py -q— 9 passedpython -m pytest tests/test_scaling -q— 9 passedflake8— passedblack --check— passedgit diff --check— passedPart of #958.