[MNT] Deprecate LogCpTransformer in favour of LogTransformer with C parameter by adityaanikam · Pull Request #970 · feature-engine/feature_engine

adityaanikam · 2026-07-24T10:48:03Z

Closes #957

Summary

Merges LogCpTransformer into LogTransformer as approved in the issue
discussion. LogTransformer gains a C parameter ("auto" / int / float /
dict) mirroring LogCpTransformer's exact semantics, defaulting to C=0 so
existing LogTransformer() usage is unchanged. LogCpTransformer becomes a
thin subclass defaulting to C="auto", noted as being consolidated into
LogTransformer in favor of LogTransformer(C=...).

Backward compatibility

LogTransformer()'s behavior and error message/timing are unchanged:
fit-time validation still fires exactly when C resolves to 0 (the new
default), with the original message.
LogCpTransformer's numerical behavior and C_ computation are unchanged.

Things worth flagging

LogCpTransformer's base= error message lost its dynamic
Got {base} instead. suffix — it now uses LogTransformer's original
plain message, since that's the class whose wording was promised
unchanged. Updated the corresponding test to match.
LogCpTransformer gets its own _more_tags()/__sklearn_tags__()
overrides resetting to the un-xfailed tags, so it doesn't silently
inherit LogTransformer's zero-value xfail exemptions that it never
needed (its "auto" default always finds a valid shift).
There's no existing convention in this codebase for deprecating a class
(searched — no precedent), so I kept the notice to a plain docstring
.. note:: rather than inventing a formal .. deprecated:: version
directive. I also didn't touch docs/user_guide/transformation/ LogCpTransformer.rst, which still describes it as fully independent —
happy to update that in this PR or a follow-up, whichever you'd prefer.

The 0-handling-without-a-declared-constant question from the issue is left
for a follow-up, per the discussion.

Testing

pytest tests/test_transformation/ — 117 passed, including a new regression
test proving the C=0 default preserves the original fail-fast contract.

…ndentation

solegalli

Hi @adityaanikam

This is looking really good! Thank you so much for the first draft.

I left a few comments, mostly to add future warnings about the deprecation, and to add the tests for C in the logtransform tests.

The last bit that needs updating to have this one ready is the user guide. Inside the docs folder, useguide/transformers, we need to add the demo of the use of C to Logtransfor.rst. You could probably re-adapt the examples that we have for logcp.

Thanks a lot!

solegalli · 2026-07-26T05:25:12Z

+        The constant C to add to the variable before the logarithm, i.e., log(x + C).
+
+        - If 0 (the default), no constant is added and the variable must be
+          strictly positive, matching the transformer's original behavior.


Suggested change

strictly positive, matching the transformer's original behavior.

strictly positive.

solegalli · 2026-07-26T05:26:06Z

-    The LogTransformer() only works with positive values. If the variable
-    contains a zero or a negative value the transformer will return an error.
+    By default, C=0, so LogTransformer() only works with positive values and
+    behaves exactly as it always has: if a variable contains a zero or a negative


Suggested change

behaves exactly as it always has: if a variable contains a zero or a negative

If a variable contains a zero or a negative

solegalli · 2026-07-26T05:26:18Z


-    The LogTransformer() only works with positive values. If the variable
-    contains a zero or a negative value the transformer will return an error.
+    By default, C=0, so LogTransformer() only works with positive values and


Suggested change

By default, C=0, so LogTransformer() only works with positive values and

By default, C=0, so LogTransformer() only works with positive values.

solegalli · 2026-07-26T05:26:46Z

-    contains a zero or a negative value the transformer will return an error.
+    By default, C=0, so LogTransformer() only works with positive values and
+    behaves exactly as it always has: if a variable contains a zero or a negative
+    value, the transformer raises an error.


Could we add a note saying that C will be changed from 0 to "auto" in version 2.1.0? I think this is the easiest way to avoid the error on negative values that we were discussing earlier.

solegalli · 2026-07-26T05:29:09Z

    {variables_}

+    C_:
+        The constant C added to each variable. Equal to C, unless C = "auto", in


Suggested change

The constant C added to each variable. Equal to C, unless C = "auto", in

The constant C added to each variable. Equal to `C`, unless `C = "auto"`, in

solegalli · 2026-07-26T05:31:08Z

-        can be applied on the selected variables, i.e., it checks that the variables
-        are positive.
+        Selects the numerical variables and, when C=0 (the default), determines
+        whether the logarithm can be applied on the selected variables, i.e., it


I think we should remove lines 157-159.

solegalli · 2026-07-26T05:36:12Z

-        The constant C to add to each variable. If C = "auto" a dictionary with
-        C = abs(min(variable)) + 1. For strictly positive variables, C = 0.
+    .. note::
+        `LogCpTransformer` is being consolidated into `LogTransformer`. New


Could we add the version changes and future deprecations in this note? Something like:

LogCpTransformer is consolidated into LogTransformer in version 2.0.0 and will be deprecated in version 2.1.0. New code....

solegalli · 2026-07-26T05:36:42Z

-class LogCpTransformer(BaseNumericalTransformer, FitFromDictMixin):
+class LogCpTransformer(LogTransformer):
    """
    LogCpTransformer() applies the transformation log(x + C), where x is the


Suggested change

LogCpTransformer() applies the transformation log(x + C), where x is the

#TODO: remove in version 2.1.0

LogCpTransformer() applies the transformation log(x + C), where x is the

solegalli · 2026-07-26T05:39:29Z

+        super().__init__(
+            variables=variables, return_empty=return_empty, base=base, C=C
        )



Could we add a future warning in the init saying that LogCP... was deprecated in favour of LogTransformer in version 2.0.0 and will be removed in version 2.1.0. Use LogTransformer() instead? So every time someone uses this class, they are alerted that it will disappear in the next version?

solegalli · 2026-07-26T05:45:58Z

    # test transform output
    pd.testing.assert_frame_equal(X, df_vartypes)
+
+


We should bring the tests for C, from test_logcp to this file as well, so we know it works as intended.

- Emit a FutureWarning on LogCpTransformer instantiation, announcing its removal in version 2.1.0 in favour of LogTransformer(C=auto). - Align the LogCpTransformer docstring note with that warning: it is deprecated in 2.0.0 and removed in 2.1.0, not deprecated in 2.1.0. - Explain why `variables` is ignored when C is a dictionary (the variables are taken from the dictionary keys) instead of restating that C accepts dictionaries, which the parameter list above already documents. - Mark the pending removal with a TODO above the class statement so it stays a developer note rather than rendered documentation text. - Note in LogTransformer's docstring that C will default to auto in 2.1.0. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Port the C-parameter tests from test_logcp_transformer.py to test_log_transformer.py, covering parameter validation, C=auto/dict/int at fit time, the transform-time positivity check, both logarithm bases and inverse_transform. Uses a `df_c` fixture to avoid colliding with the existing conftest fixtures. Document the new parameter in the user guide: replace the note stating that non-positive values always raise and pointing to the now-deprecated LogCpTransformer, and add a section demonstrating C=auto on the diabetes dataset. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

adityaanikam · 2026-07-26T13:39:29Z

Hi @solegalli, just wanted to flag something separate from the code review. CI has failed in the same way on three different runs of this branch now. Most recently, test_feature_engine_py313 and test_feature_engine_py314 failed on 7eae67fd, each with the same result:

51 failed, 1969 passed

All 51 failures seem to come from fetch_california_housing() failing with:

sklearn.datasets._base._fetch_remote → urlretrieve → urllib.error.HTTPError: HTTP Error 403: Forbidden

That accounts for 48 failures in test_wrappers/test_sklearn_wrapper.py, 2 in test_discretisation, and 1 in test_selection/test_mrmr.py. They’re all tests that fetch the California housing dataset, and none of the failures are in test_transformation.

The Python version that fails also seems to vary between runs py310 on one, py314 on another, and py313 + py314 most recently so it looks more like a flaky download/CI issue than something caused by this change.

I had a look at the dataset URL as well. It redirects to a presigned S3 URL with X-Amz-Expires=10, so the signature only has a 10 second window. I wonder if a runner occasionally takes too long to follow the redirect and ends up hitting an expired URL. The download works consistently for me locally.

Since fetch_california_housing() caches the dataset under ~/scikit_learn_data after the first successful download, caching that directory in CircleCI might avoid this altogether.

Happy to look into that if it would be useful, but wanted to flag it first in case you’re already aware of it or have a preferred way of handling it.

solegalli

Hi @adityaanikam

This is almost ready, thank you so much!

A few more things are still needed:

We need to remove LogCpTransformer from the following files:

README.md:122 — plain bullet list entry
docs/index.rst:240 — API summary table
docs/user_guide/transformation/index.rst:31,52 — comparison table + toctree
docs/user_guide/transformation/PowerTransformer.rst:77 — "see also" list
docs/api_doc/transformation/index.rst:13 — API toctree

While you are at it, please add that the LogTransformer now adds, optionally a constant if relevant. For example in the table (not sure it fits, I don't remember what the table says at the moment).

We also need a test for LogCP to corroborate that the future warning is being triggered.

Finally, we need a note in this file docs/user_guide/transformation/LogCpTransformer.rst mentioning that it was deprecated in version 2.0.0 and will be removed in version ...

After that, we are ready to merge!

Could you take a look at this and other comments that I left in the files themselves?

solegalli · 2026-07-28T08:16:15Z

 .. note::

-    Note that the logarithm can only be applied to positive values. Thus, if the variable contains 0 or negative values, this transformer will return an error.
+    By default (``C=0``), the logarithm can only be applied to positive values. If the variable contains 0 or negative values, the transformer will return an error, unless you use the ``C`` parameter described below.


Suggested change

By default (``C=0``), the logarithm can only be applied to positive values. If the variable contains 0 or negative values, the transformer will return an error, unless you use the ``C`` parameter described below.

The logarithm can only be applied to positive values; if the variable contains 0 or negative values, the transformer will raise an error. To avoid this, add a constant to the variables by using the `C` parameter (more details below).

solegalli · 2026-07-28T08:17:49Z

+    By default (``C=0``), the logarithm can only be applied to positive values. If the variable contains 0 or negative values, the transformer will return an error, unless you use the ``C`` parameter described below.

-To transform non-positive variables you can add a constant to shift the data points towards positive values. You can do this by using :class:`LogCpTransformer()`.
+To transform non-positive variables, pass a value to the ``C`` parameter to shift


Suggested change

To transform non-positive variables, pass a value to the ``C`` parameter to shift

solegalli · 2026-07-28T08:17:59Z


-To transform non-positive variables you can add a constant to shift the data points towards positive values. You can do this by using :class:`LogCpTransformer()`.
+To transform non-positive variables, pass a value to the ``C`` parameter to shift
+the data points towards positive values, as shown in the section below.


Suggested change

the data points towards positive values, as shown in the section below.

We can remove these sentences, since it's included in the note above.

solegalli · 2026-07-28T08:19:41Z



+Transforming non-positive variables with C
+------------------------------------------


We need to replace the lines under the hading -- with sublevel heading so ˜˜˜, so that this heading appears under the heading Python implementaiton

solegalli · 2026-07-28T08:20:39Z

+.. code:: python
+
+    from sklearn.model_selection import train_test_split
+    from sklearn.datasets import load_diabetes


this is a nitpick, but since we are editing still, it's nicer to have imports in alphabetical order, so datasets, before model_selection

solegalli · 2026-07-28T08:33:48Z

@@ -28,7 +28,7 @@ def test_c_parameter(c):

 @pytest.mark.parametrize("c", ["string", [1, 2]])
 def test_c_raises_error(c):


we need to add a test for the future deprecation warning to this file

adityaanikam · 2026-07-28T18:06:34Z

Hi @solegalli, pushed everything from this round removed LogCpTransformer from the 5 listing files, updated the comparison table, applied the note rewrite/heading/import fixes on LogTransformer.rst, added the deprecation warning note to LogCpTransformer.rst, and added the FutureWarning test to test_logcp_transformer.py.

One CI issue along the way, for the record: removing LogCpTransformer from the toctrees while keeping its .rst pages (per your comment) left those two pages unreachable from any toctree, which sphinx-build -W treats as a hard error. Fixed by adding :orphan: to both docs/api_doc/transformation/LogCpTransformer.rst and docs/user_guide/transformation/LogCpTransformer.rst verified with a full clean local build (213/213 pages, no warnings) before pushing.

Two small things from your earlier round I want to flag explicitly rather than let slide through silently, since you said this was close to merge ready:

The LogCpTransformer note currently says "deprecated in 2.0.0, will be removed in 2.1.0" rather than your literal suggestion ("consolidated in 2.0.0 and will be deprecated in 2.1.0"). I went with this wording because it's the one consistent with the actual FutureWarning message, which fires now (in 2.0.0) rather than waiting until 2.1.0 so the class is already deprecated, not "about to be." Happy to change it if you'd rather phrase it differently.
The # TODO: remove in version 2.1.0 sits as a plain comment above the class statement rather than inside the docstring where your suggestion diff placed it. I kept it outside because a docstring is the API documentation Sphinx renders — inside it, that line would show up as user-facing text on the published docs page, which I don't think was the intent. Let me know if you did want it in the docstring for some reason I'm missing.

adityaanikam added 8 commits July 24, 2026 16:06

Merge LogCpTransformer into LogTransformer with C parameter

2dcb068

Merge LogCpTransformer into LogTransformer with C parameter

dc3cc3c

Fix style: trailing newline, blank line spacing, and note directive i…

07aca63

…ndentation

Fix style: trailing newline, blank line spacing, and note directive i…

e5024f8

…ndentation

Fix style: trailing newline, blank line spacing, and note directive i…

17ca81a

…ndentation

Fix E302: add missing blank line before new test

b34011e

Fix W293: remove trailing whitespace from blank line

fd17522

Normalize log.py line endings to LF, matching repo convention

bf16b34

solegalli reviewed Jul 26, 2026

View reviewed changes

solegalli changed the title ~~Merge LogCpTransformer into LogTransformer with C parameter~~ [MNT] Deprecate LogCpTransformer in favour of LogTransformer with C parameter Jul 26, 2026

adityaanikam and others added 4 commits July 26, 2026 12:44

Add the offending value to the base validation error message

2b5c9b9

Include dictionaries in the C validation error message

7eae67f

solegalli reviewed Jul 28, 2026

View reviewed changes

adityaanikam added 2 commits July 28, 2026 23:07

Remove LogCpTransformer from docs listings and document the C parameter

9d579d1

Mark orphaned LogCpTransformer doc pages to satisfy Sphinx

609ef81

	strictly positive, matching the transformer's original behavior.
	strictly positive.

	behaves exactly as it always has: if a variable contains a zero or a negative
	If a variable contains a zero or a negative

	By default, C=0, so LogTransformer() only works with positive values and
	By default, C=0, so LogTransformer() only works with positive values.

	The constant C added to each variable. Equal to C, unless C = "auto", in
	The constant C added to each variable. Equal to `C`, unless `C = "auto"`, in

	LogCpTransformer() applies the transformation log(x + C), where x is the
	#TODO: remove in version 2.1.0
	LogCpTransformer() applies the transformation log(x + C), where x is the

		# test transform output
		pd.testing.assert_frame_equal(X, df_vartypes)

	By default (``C=0``), the logarithm can only be applied to positive values. If the variable contains 0 or negative values, the transformer will return an error, unless you use the ``C`` parameter described below.
	The logarithm can only be applied to positive values; if the variable contains 0 or negative values, the transformer will raise an error. To avoid this, add a constant to the variables by using the `C` parameter (more details below).



		Transforming non-positive variables with C
		------------------------------------------

		@@ -28,7 +28,7 @@ def test_c_parameter(c):

		@pytest.mark.parametrize("c", ["string", [1, 2]])
		def test_c_raises_error(c):

Uh oh!

Conversation

adityaanikam commented Jul 24, 2026

Summary

Backward compatibility

Things worth flagging

Testing

Uh oh!

solegalli left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adityaanikam commented Jul 26, 2026

Uh oh!

solegalli left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adityaanikam commented Jul 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants