Skip to content
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

LogEndogTransformer doesn't work in pipelines #407

Closed
jseabold opened this issue Jan 19, 2021 · 2 comments · Fixed by #408
Closed

LogEndogTransformer doesn't work in pipelines #407

jseabold opened this issue Jan 19, 2021 · 2 comments · Fixed by #408
Labels
🪲 : bug good first issue A good opportunity for first-time contributors.

Comments

@jseabold
Copy link

Describe the bug

When cloning a transformer, it gets the parameter names from the __init__. When using LogEndogTransformer, you never set lmbda, you set lmbda2 for the Box-Cox transformer, so when you clone the transformer, it thinks the values of lmbda is 0 even though you may have set it to something else.

To Reproduce
Steps to reproduce the behavior:

In [3]: from pmdarima.preprocessing.endog import LogEndogTransformer

In [4]: from sklearn.base import clone

In [5]: LogEndogTransformer(lmbda=10).fit_transform([0, 10])
Out[5]: (array([2.3026, 2.9957]), None)

In [6]: clone(LogEndogTransformer(lmbda=10)).fit_transform([0, 10])
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-6-360cffae9200> in <module>
----> 1 clone(LogEndogTransformer(lmbda=10)).fit_transform([0, 10])

~/src/pmdarima/pmdarima/preprocessing/base.py in fit_transform(self, y, X, **kwargs)
     55         """
     56         self.fit(y, X, **kwargs)  # TODO: eventually do not pass kwargs to fit
---> 57         return self.transform(y, X, **kwargs)
     58
     59     @abc.abstractmethod

~/src/pmdarima/pmdarima/preprocessing/endog/log.py in transform(self, y, X, **transform_kwargs)
     73             The exog array
     74         """
---> 75         return super().transform(y, X, **transform_kwargs)
     76
     77     def inverse_transform(self, y, X=None, **kwargs):  # TODO: kwargs go away

~/src/pmdarima/pmdarima/preprocessing/endog/boxcox.py in transform(self, y, X, **kwargs)
    123             msg = "Negative or zero values present in y"
    124             if action == "raise":
--> 125                 raise ValueError(msg)
    126             elif action == "warn":
    127                 warnings.warn(msg, UserWarning)

ValueError: Negative or zero values present in y

In [7]: clone(LogEndogTransformer(lmbda=10))
Out[7]: LogEndogTransformer(floor=1e-16, lmbda=0, neg_action='raise')

Versions

I'm running a development version to work around a few issues such as this one. Tag is 23d4d0d.

System:
    python: 3.7.6 | packaged by conda-forge | (default, Mar  5 2020, 15:03:29)  [Clang 9.0.1 ]
executable: /Users/skipper/.miniconda3/bin/python
   machine: Darwin-19.6.0-x86_64-i386-64bit

Python dependencies:
        pip: 20.3.3
 setuptools: 49.6.0.post20201009
    sklearn: 0.22
statsmodels: 0.12.1
      numpy: 1.19.4
      scipy: 1.4.1
     Cython: 0.29.17
     pandas: 1.1.3
     joblib: 1.0.0
   pmdarima: 0.0.0

If I can work out a patch quickly, I'll make a PR, but not sure how to work around this off the top of my head.

@jseabold
Copy link
Author

Well, this is the easiest workaround. Not thoroughly tested.

diff --git a/pmdarima/preprocessing/endog/log.py b/pmdarima/preprocessing/endog/log.py
index 5ad8e8ed..495ed5cb 100644
--- a/pmdarima/preprocessing/endog/log.py
+++ b/pmdarima/preprocessing/endog/log.py
@@ -101,3 +101,25 @@ class LogEndogTransformer(BoxCoxEndogTransformer):
             The inverse-transformed exogenous array
         """
         return super().inverse_transform(y, X, **kwargs)
+
+    def get_params(self, deep=True):
+        """
+        Get parameters for this estimator.
+
+        Parameters
+        ----------
+        deep : bool, default=True
+            If True, will return the parameters for this estimator and
+            contained subobjects that are estimators.
+
+        Returns
+        -------
+        params : mapping of string to any
+            Parameter names mapped to their values.
+        """
+        # this method is shadowed because we shadow the the lmbda parameter
+        # name. Instead of setting lmbda, this transformer stores the parameter
+        # as lmbda2
+        params = super().get_params(deep=deep)
+        params['lmbda'] = self.lmbda2
+        return params

@tgsmith61591
Copy link
Member

Thanks @jseabold great report. We will get this fixed up

@tgsmith61591 tgsmith61591 added the good first issue A good opportunity for first-time contributors. label Jan 20, 2021
tgsmith61591 added a commit that referenced this issue Jan 20, 2021
tgsmith61591 added a commit that referenced this issue Jan 20, 2021
tgsmith61591 added a commit that referenced this issue Jan 20, 2021
* Address Issue #407

* Add a test with log endog transformer

* 📚 Update whats_new

* 📚 Update whats_new
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🪲 : bug good first issue A good opportunity for first-time contributors.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants