Skip to content

ENH improve HTML display of FunctionTransformer#29158

Merged
thomasjpfan merged 9 commits intoscikit-learn:mainfrom
Charlie-XIAO:ft-display-name
Jul 21, 2024
Merged

ENH improve HTML display of FunctionTransformer#29158
thomasjpfan merged 9 commits intoscikit-learn:mainfrom
Charlie-XIAO:ft-display-name

Conversation

@Charlie-XIAO
Copy link
Contributor

@Charlie-XIAO Charlie-XIAO commented Jun 2, 2024

Fixes #29032.

Main changes explained

  • Add a caption field to the label which is the smaller, grayed text shown below the name.
  • Replace FunctionTransformer in the name field by the function name and display the estimator name in caption; the function name is est.func.__name__, or est.func.func.__name__ (for functools.partial), or {est.func.__class__.__name__}(...).
  • Allow Documentation to {xxx} tooltip to show something other than xxx=name; this is needed in this case because name is not the estimator name now.
  • Change the label to flex layout which seems to me more suitable and flexible in this case

This is more or less still a draft solution so discussions/suggestions welcome.

Testing this PR

In a notebook:

from functools import partial

import numpy as np
from sklearn.pipeline import make_pipeline
from sklearn.preprocessing import FunctionTransformer

def function_one(x, y, z):
    return x + y + z

pipe = make_pipeline(
    FunctionTransformer(lambda x: x + 1),
    FunctionTransformer(function_one),
    FunctionTransformer(partial(function_one, y=1)),
    FunctionTransformer(partial(partial(function_one, y=1), z=2)),
    FunctionTransformer(np.vectorize(function_one)),
    FunctionTransformer(np.vectorize(partial(function_one, y=1))),
)
pipe

The above would render:

image

@github-actions
Copy link

github-actions bot commented Jun 2, 2024

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: 77d2a24. Link to the linter CI: here

@adrinjalali
Copy link
Member

I like the idea!

@Charlie-XIAO
Copy link
Contributor Author

I'm not sure why codecov is failing for this PR. The only uncovered lines in the report did not seem related to this PR, or should I fix the partially covered lines as well?

Copy link
Member

@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM.

@adrinjalali adrinjalali added the Waiting for Second Reviewer First reviewer is done, need a second one! label Jul 4, 2024
@Charlie-XIAO
Copy link
Contributor Author

Charlie-XIAO commented Jul 19, 2024

@timvink @glemaitre Would you want to take a look at this one?

@timvink
Copy link
Contributor

timvink commented Jul 19, 2024

Yes this look great !

@adrinjalali
Copy link
Member

Merge conflict, also, maybe @lesteve could have a look here?

@Charlie-XIAO
Copy link
Contributor Author

Changelog conflict, resolved :)

Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small nit, otherwise LGTM

@Charlie-XIAO
Copy link
Contributor Author

Thanks for the review @thomasjpfan; comment resolved in 77d2a24

@thomasjpfan thomasjpfan merged commit 8eafd10 into scikit-learn:main Jul 21, 2024
@Charlie-XIAO Charlie-XIAO deleted the ft-display-name branch July 21, 2024 14:42
MarcBresson pushed a commit to MarcBresson/scikit-learn that referenced this pull request Sep 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improve FunctionTransformer diagram representation

4 participants