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

Add infer_feature_types to start.ipynb #1700

Merged
merged 9 commits into from
Jan 19, 2021
Merged

Add infer_feature_types to start.ipynb #1700

merged 9 commits into from
Jan 19, 2021

Conversation

bchen1116
Copy link
Contributor

@bchen1116 bchen1116 commented Jan 15, 2021

fix #1667

Updated docs here

@bchen1116 bchen1116 self-assigned this Jan 15, 2021
@codecov
Copy link

codecov bot commented Jan 15, 2021

Codecov Report

Merging #1700 (a2bec19) into main (7fc9783) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1700   +/-   ##
=======================================
  Coverage   100.0%   100.0%           
=======================================
  Files         242      242           
  Lines       18942    18942           
=======================================
  Hits        18934    18934           
  Misses          8        8           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7fc9783...a2bec19. Read the comment docs.

Copy link
Contributor

@angela97lin angela97lin left a comment

Choose a reason for hiding this comment

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

Left comments on how to clean things up a bit, otherwise LGTM! I'm curious about the runtime and the performance though, and haven't looked at docs. Let's confirm that doc builds don't take too much longer (since fraud is more involved this is likely), and that we don't get terrible results (I just have #616) in mind.

docs/source/start.ipynb Show resolved Hide resolved
docs/source/start.ipynb Outdated Show resolved Hide resolved
Copy link
Contributor

@chukarsten chukarsten left a comment

Choose a reason for hiding this comment

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

+1 for Angela's suggestions

Copy link
Contributor

@ParthivNaresh ParthivNaresh left a comment

Choose a reason for hiding this comment

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

Everything looks good! Just a little change to the markdown to provide some clarity.

docs/source/start.ipynb Show resolved Hide resolved
@bchen1116
Copy link
Contributor Author

@angela97lin the doc builds (here, here, and here) for this PR take than a little longer than other PRs (here and here) but not much longer. I think it should be fine to merge.

Copy link
Contributor

@angela97lin angela97lin left a comment

Choose a reason for hiding this comment

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

Yup, build time doesn't seem too much longer! Added a few more comments but otherwise 🚢

docs/source/start.ipynb Outdated Show resolved Hide resolved
docs/source/start.ipynb Outdated Show resolved Hide resolved
@bchen1116 bchen1116 merged commit c321cae into main Jan 19, 2021
@bchen1116 bchen1116 mentioned this pull request Jan 26, 2021
@freddyaboulton freddyaboulton deleted the bc_1667_infer branch May 13, 2022 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add infer_feature_types to start guide
4 participants