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

Typos + Review #13

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Typos + Review #13

wants to merge 1 commit into from

Conversation

lucyleeow
Copy link

@lucyleeow lucyleeow commented Nov 13, 2019

PR consists mostly of typos.

I can also make the rendered notebooks (what is your procedure for doing this?)

Some suggestions:

01_adult_census_exploration:

  • adult_census.profile_report() told us that there are a few duplicate rows. It may be worthwhile explaining how these duplicate entries may affect/not affect prediction?

02_basic_preprocessing

  • convergence warning - you explain that this tells us that our model stopped learning because it reached the maximum number of iterations allowed and that scaling the data will help. Can you expand on what convergence means, why increasing the number of allowed iterations is a bad idea and why scaling the data helps?
  • explain what the StandardScaler does? Maybe not everyone knows the equation?

03_basic_parameters_tuning

  • I think you need to explain more about the hyper-parameter C. Maybe even just give them a useful link to read on regularisation and overfitting?
  • For the last cell:
model = make_pipeline(
    preprocessor, LogisticRegressionCV(max_iter=1000, solver='lbfgs', cv=5)
)
score = cross_val_score(model, data, target, n_jobs=4, cv=5)

you don't provide a Cs argument like you do above and it might be worth mentioning that by default it tests a grid of 10 C values.

@lucyleeow lucyleeow changed the title Review Typos + Review Nov 13, 2019
@glemaitre
Copy link
Collaborator

I will port the typo for the 4th notebook and correct the other notebook a little later.

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.

2 participants