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.

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

[AIR] Improve preprocessor documentation #27215

Merged
merged 88 commits into from
Aug 11, 2022
Merged

[AIR] Improve preprocessor documentation #27215

merged 88 commits into from
Aug 11, 2022

Conversation

bveeramani
Copy link
Member

@bveeramani bveeramani commented Jul 28, 2022 •
edited
Loading

Why are these changes needed?

User Guide Changes
We've implemented many preprocessors, and it's unclear which you should use. I've added a section that describes when you should use each preprocessor.

API Reference Changes

The preprocessor reference has problems:

  • Examples are broken or non-existent.
  • Explanations are minimal or confusing.
  • Preprocessors aren't organized in a useful way.

This PR:

  • Reorganizes the preprocessor reference.
  • Adds tested examples for every built-in preprocessor.
  • Clarifies and elaborates every preprocessor's description

In addition, this PR also

  • Adds cross-references.
  • Adds seealso sections.
  • Adds Latex math (where appropriate)

Related issue number

Checks

  • I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

@bveeramani bveeramani changed the title [AIR] Improve LabelEncoder docstring [AIR] Improve encoder docstrings Jul 28, 2022
@bveeramani bveeramani marked this pull request as draft July 28, 2022 20:54
@richardliaw
update-preprocessors
05d1e8f 
Signed-off-by: Richard Liaw <rliaw@berkeley.edu>
@richardliaw richardliaw self-assigned this Aug 9, 2022
clarkzinzow
clarkzinzow approved these changes Aug 9, 2022
View reviewed changes
Copy link
Contributor

@clarkzinzow clarkzinzow left a comment

Choose a reason for hiding this comment

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

LGTM!

richardliaw
richardliaw reviewed Aug 9, 2022
View reviewed changes
python/ray/data/preprocessors/batch_mapper.py
Comment on lines +33 to +34
>>> def fn(batch: pd.DataFrame) -> pd.DataFrame:
... return batch.drop("Y", axis="columns")
Copy link
Contributor

Choose a reason for hiding this comment

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

nit; ideally we choose an example that is actually not supported by Ray

Copy link
Member Author

Choose a reason for hiding this comment

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

How do you drop a column without BatchMapper?

richardliaw
richardliaw reviewed Aug 9, 2022
View reviewed changes
python/ray/data/preprocessors/concatenator.py
will be preserved.
This preprocessor concatenates numeric columns and stores the result in a new
column. The new column contains
:class:`~ray.air.util.tensor_extensions.pandas.TensorArrayElement` objects of
Copy link
Contributor

Choose a reason for hiding this comment

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

seems like TensorArrayElement is not a documented class?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not. If we add TensorArrayElement to the data reference in a future PR, this link will work.

richardliaw
richardliaw reviewed Aug 9, 2022
View reviewed changes
python/ray/data/preprocessors/encoder.py
0 Shaolin Soccer [1, 1, 1]
1 Moana [1, 1, 0]
2 The Smartest Guys in the Room [0, 0, 0]
>>> encoder.stats_ # doctest: +SKIP
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we document this attribute explicitly?

Copy link
Member Author

Choose a reason for hiding this comment

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

Are we including Preprocessor attributes in the public interface? Talked to @matthewdeng and he mentioned it's currently undefined.

I added stats_ to this example because, without it, there's no way to know which categories are used (max_categories doesn't encode infrequent categories).

Copy link
Member Author

@bveeramani bveeramani Aug 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Also, if we're added an attribute to the public interface, we should add categories_ or infrequent_categories_. stats_ looks weird. See #27357

@richardliaw
update-starter-text
fe2c9a3 
Signed-off-by: Richard Liaw <rliaw@berkeley.edu>
@richardliaw
Copy link
Contributor

Let's merge #27610 into this to make cherrypicking easier.

@bveeramani bveeramani mentioned this pull request Aug 10, 2022
Closed
7 tasks
@bveeramani bveeramani changed the title [AIR] Improve preprocessor reference [AIR] Improve preprocessor documentation Aug 10, 2022
@richardliaw richardliaw merged commit 7da7dbe into ray-project:master Aug 11, 2022
@bveeramani bveeramani deleted the bveeramani/label-encoder-docstring branch August 11, 2022 01:35
bveeramani added a commit to bveeramani/ray that referenced this pull request Aug 11, 2022
@bveeramani @matthewdeng @richardliaw
[AIR] Improve preprocessor documentation (ray-project#27215)
56f2390 
Co-authored-by: matthewdeng <matthew.j.deng@gmail.com>
Co-authored-by: Richard Liaw <rliaw@berkeley.edu>
@bveeramani bveeramani mentioned this pull request Aug 11, 2022
Merged
7 tasks
Stefan-1313 pushed a commit to Stefan-1313/ray_mod that referenced this pull request Aug 18, 2022
@bveeramani @matthewdeng @richardliaw
[AIR] Improve preprocessor documentation (ray-project#27215)
d32249c 
Co-authored-by: matthewdeng <matthew.j.deng@gmail.com>
Co-authored-by: Richard Liaw <rliaw@berkeley.edu>
Signed-off-by: Stefan van der Kleij <s.vanderkleij@viroteq.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richardliaw richardliaw richardliaw approved these changes

@matthewdeng matthewdeng matthewdeng approved these changes

@clarkzinzow clarkzinzow clarkzinzow approved these changes

@ericl ericl Awaiting requested review from ericl

@scv119 scv119 Awaiting requested review from scv119

@jjyao jjyao Awaiting requested review from jjyao

@jianoaix jianoaix Awaiting requested review from jianoaix

@maxpumperla maxpumperla Awaiting requested review from maxpumperla

@pcmoritz pcmoritz Awaiting requested review from pcmoritz

@stephanie-wang stephanie-wang Awaiting requested review from stephanie-wang

@dmatrix dmatrix Awaiting requested review from dmatrix

@sriram-anyscale sriram-anyscale Awaiting requested review from sriram-anyscale

Assignees

@matthewdeng matthewdeng

@richardliaw richardliaw

Labels
None yet
Projects
None yet
Milestone
AIR 2.0 docs
Development

Successfully merging this pull request may close these issues.
5 participants
@bveeramani @richardliaw @matthewdeng @clarkzinzow @stephanie-wang