8000 DOC Document `config_context` and transform output by ravwojdyla · Pull Request #25289 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

DOC Document config_context and transform output #25289

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 5, 2023
Merged

DOC Document config_context and transform output #25289

merged 1 commit into from
Jan 5, 2023

Conversation

ravwojdyla
Copy link
Contributor

Reference Issues/PRs

Fixes: #25287

What does this implement/fix? Explain your changes.

Add documentation around config_context and transform output usage.

@ravwojdyla ravwojdyla force-pushed the doc-config-cntx branch 2 times, most recently from f4c9388 to 82fcc8a Compare January 4, 2023 15:13
@ravwojdyla ravwojdyla mentioned this pull request Jan 4, 2023
Closed
betatim
betatim reviewed Jan 4, 2023
View reviewed changes
betatim
betatim reviewed Jan 4, 2023
View reviewed changes
@betatim
Copy link
Member
betatim commented Jan 4, 2023

Thanks for taking care of this. I left two comments, mostly around rephrasing things. Let me know what you think of the suggestions.

@ravwojdyla ravwojdyla force-pushed the doc-config-cntx branch 2 times, most recently from eb55296 to 3eebf43 Compare January 4, 2023 16:06
@ravwojdyla
Copy link
Contributor Author

@betatim thanks for the review - updated.

betatim
betatim approved these changes Jan 5, 2023
View reviewed changes
Copy link
Member
@betatim betatim left a comment

Choose a reason for hiding this comment

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

LGTM

@betatim betatim added the Quick Review For PRs that are quick to review label Jan 5, 2023
glemaitre
glemaitre reviewed Jan 5, 2023
View reviewed changes
examples/miscellaneous/plot_set_output.py Outdated
Comment on lines 129 to 134
with config_context(transform_output="pandas"):
# X_test_scaled_pd will contain a dataframe
X_test_scaled_pd = scaler.transform(X_test[num_cols])

# X_test_scaled_np will contain a numpy array
X_test_scaled_np = scaler.transform(X_test[num_cols])
Copy link
Member

Choose a reason for hiding this comment

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

I would find it better to show the output directly:

Suggested change
with config_context(transform_output="pandas"):
# X_test_scaled_pd will contain a dataframe
X_test_scaled_pd = scaler.transform(X_test[num_cols])
# X_test_scaled_np will contain a numpy array
X_test_scaled_np = scaler.transform(X_test[num_cols])
# %%
with config_context(transform_output="pandas"):
scaler.transform(X_test[num_cols])
# %%
scaler.transform(X_test[num_cols])

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good to me, I want to point out that the suggested change:

  • will not print the pandas output, since it's inside the context manager
  • the numpy output is relatively verbose

See:

image

@glemaitre we could instead do a head() of the scaled values for the dataframe, and maybe slice for the numpy array? would that be good?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is how it would look like:

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pushed this ^ change in the latest version 1f4b855

Copy link
Member

Choose a reason for hiding this comment

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

Good idea to show the content and the use of head(). I like it

Copy link
Member

Choose a reason for hiding this comment

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

Yes this would be nice. Thinking about it maybe reverting a small code comment to mention array/dataframe could be better.

jjerphan
jjerphan approved these changes Jan 5, 2023
View reviewed changes
Copy link
Member
@jjerphan jjerphan left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM given that https://github.com/scikit-learn/scikit-learn/pull/25289/files#r1062343534 's remark is taken into account.

@jjerphan jjerphan removed the Quick Review For PRs that are quick to review label Jan 5, 2023
glemaitre
glemaitre reviewed Jan 5, 2023
View reviewed changes
Copy link
Member
@glemaitre glemaitre left a comment

Choose a reason for hiding this comment

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

Feel free to modify the wording.

glemaitre
glemaitre reviewed Jan 5, 2023
View reviewed changes
Copy link
Member
@glemaitre glemaitre left a comment

Choose a reason for hiding this comment

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

LGTM. Enabling merging when CIs will be ready.

ogrisel
ogrisel approved these changes Jan 5, 2023
View reviewed changes
Copy link
Member
@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

LGTM as well, apparently the auto-merge thingy did not work. I will squash and merge manually.

@ogrisel ogrisel merged commit 3efdc89 into scikit-learn:main Jan 5, 2023
@ravwojdyla ravwojdyla deleted the doc-config-cntx branch January 6, 2023 16:54
jjerphan pushed a commit to jjerphan/scikit-learn that referenced this pull request Jan 20, 2023
@ravwojdyla @jjerphan
Document config_context and transform output (scikit-learn#25289)
8cfead0
jjerphan pushed a commit to jjerphan/scikit-learn that referenced this pull request Jan 20, 2023
@ravwojdyla @jjerphan
Document config_context and transform output (scikit-learn#25289)
ea84871
jjerphan pushed a commit to jjerphan/scikit-learn that referenced this pull request Jan 23, 2023
@ravwojdyla @jjerphan
Document config_context and transform output (scikit-learn#25289)
be169bf
adrinjalali pushed a commit that referenced this pull request Jan 24, 2023
@ravwojdyla @adrinjalali
Document config_context and transform output (#25289)
d6d96d2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@betatim betatim betatim approved these changes

@glemaitre glemaitre glemaitre left review comments

@ogrisel ogrisel ogrisel approved these changes

@jjerphan jjerphan jjerphan approved these changes

Assignees
No one assigned
Labels
Documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

transform_output set in config_context not preserved in the Transformer object?
5 participants
@ravwojdyla @betatim @ogrisel @glemaitre @jjerphan
0