Closed joelostblom closed 1 year ago
Downgrade to pandas <1.5 to avoid warning until next Altair release FutureWarning: iteritems is deprecated and will be removed in a future version. Use .items instead.
I'm just going to add code to suppress warnings rather than muck with package versions. We can remove that code later once altair upgrades.
np.random.seed is not recommended anymore Instead we shuold create a random state object and use it methods
I thought about this for a bit, and tried to implement both ways. I'm going to stick with np.random.seed
.
Yes, it does have the drawback that other code may muck with the global seed, and you don't really know whether functions you're using are using the numpy
default rng.
BUT for the purposes of students learning about how randomness in computers works as part of a 100-level course, it's just so much simpler and less error prone for students to teach them about setting a global seed.
Given that the drawbacks of np.random.seed
only appear in parallel code and/or when using packages outside the course, I'm going to stick with np.random.seed
.
That being said, I will add a notebox to tell students about the two caveats I mentioned above, and about RandomState
objects.
However, I don't think we will ever need to use np.random.seed. With both sklearn methods and pandas sample we will always control randomness via the random_state parameter.
We shouldn't be setting the random_state
manually throughout an analysis. Generally, most common PRNGs (mersenne, xoshiro and variants, pcg, etc) only work properly when they are allowed to run as a long chain.
I think it is more confusing than helpful to also talk about seeds here, and we can just show the effect of a different random_state when doing the train_test split instead.
Since we need to set the seed once at the start of the analysis, we need to teach them about what seeding does.
(They also see it a tonne in their worksheets anyway, so at least it isn't totally foreign to them).
We haven't taught how to use a lambda function like this. We could since it is useful and very flexible, or if we think it is too complex, we could teach how to perform the same operation either via cancer["Class"].map({'M': 'Malignant', 'B': 'Benign'}) (which would be convenient but a new thing) or simply filtering: cancer.loc[cancer['Class'] == 'M', 'Class'] = 'Malignant' + the benign version (which is less elegant but builds on what they have learned already)
@joelostblom I've been using .replace
instead of .map
in Ch5 because apparently map
puts NaNs
when it doesn't find a key -- see example here https://stackoverflow.com/questions/62947285/is-there-a-difference-between-series-replace-and-series-map-in-pandas . I think the default behaviour of replace
is nicer (lazy coding :) ). I'll plan to do this in Ch6 too.
Could we just call it "model" instead of "model specification" everywhere?
Yes, that's a hold over from tidymodels. I'm using the terminology "model object"
We talk about "majority classifier" but we never show how to create one. This is really easy to setup in sklearn and it is a useful strategy for students to learn so that they can compare against a baseline. I suggest we show them how to setup a dummy classifier (and a dummy regressor in later chapters). We might also consider renaming "majority classifier" to "dummy classifier" in the text so not be ambiguous
I like this idea, but it's more of a "nice to have". I'll open an issue and punt for later.
Do we ever even need to show confusion_matrix
I'm just going to use pd.crosstab
. It's cleaner.
We don't show how we create the irrelevant columns
We explain how they're created in the paragraph above. I don't think we need to show them the code for this because they shouldn't ever be adding random columns in practice.
We need to explain confusion matrices and what is in each square, there are resources in BAIT 509 and DSCI 573 for this.
It's good enough for now, punted to later as an issue
The code in the cross-validation section repeats some of the things that were done earlier in the chapter like the train_test_split
This is where we illustrate what cross val does manually (by manually generating 5 splits). We only show the first split. You'll notice we are splitting the cancer_train
data now.
"Forward selection in Python"
It should be "in ____" because the previous section covers the conceptual stuff. I'll change to to "in scikit-learn", because thankfully it actually implements it, unlike tidymodels in R where we had to code it ourselves.
"We can then aggregate the mean and standard deviation". Again Search and replace in the entire chapter.
Note, crucially, we want the standard error here, not standard deviation. The sem
function should be used. I'm fixing that.
"yields a high cross-validation accuracy
estimatethat"
It is an estimate -- I'm keeping that word
I think we should introduce grid search and randomized search one at a time
I don't think there's any real value in introducing two separate methods for this in this class. I'm just going to introduce grid CV.
Do we explain n_jobs=-1?
Nope and it isn't necessary; removing
Relating to step 6 and 7, GridSearchCV already provides us with the best model as an attribute, granted hat we still want them to look at the plot, is there value in teaching how to extract the already fit best model from GridSearchCV.bestestimator instead of refitting it (which is redundant and potentially time consuming).
I think this is a good thing to consider later (nice to have, not necessary right now). Will spin into its own issue.
In general I think this section is not that well-written and we should go through it and improve the language as well as how the different code snippets are introduced.
I agree. I'm going to comment it out for now entirely and reintroduce it later once we have breathing room. Issue posted.
I will start going through Ch6 now, might not finish until tomorrow. A few comments on what you wrote above:
We shouldn't be setting the random_state manually throughout an analysis. Generally, most common PRNGs (mersenne, xoshiro and variants, pcg, etc) only work properly when they are allowed to run as a long chain.
I am not familiar with the internals for how the PRNGSs work, but I think both pandas and sklearn recommend the use of random_state
repeatedly over setting seeds globally, so there might be something to account for what you mention in those packages?
I've been using .replace instead of .map...
Great!
It is an estimate -- I'm keeping that word
I agree that the cross-validation is an estimate of the test accuracy. I read that sentence as that we are getting an estimate of the cross-validation accuracy, which we are not, since we are getting the actual cross-validation accuracy. But I'm not a native speaker so if it reads well to you then let's keep it.
sample
we will always control randomness via therandom_state
parameter.random_state
when doing thetrain_test
split instead.cancer["Class"].map({'M': 'Malignant', 'B': 'Benign'})
(which would be convenient but a new thing) or simply filtering:cancer.loc[cancer['Class'] == 'M', 'Class'] = 'Malignant'
+ the benign version (which is less elegant but builds on what they have learned already)FutureWarning: iteritems is deprecated and will be removed in a future version. Use .items instead.
stratify
andshuffle
, not just mention what they do (include a short example for each in a sentence or so).info
method always prints columns vertically, not just when there is a large number of them.groupby
+count
here. That creates a count for each column, so it doesn't generalize well. Instead we should use.groupby
+size
OR just usevalue_counts(normalize=True)
which means we don't have to divide by the length after (btw we should get dataframe length viadf.shape[0]
not usinglen
(applies to the entire book)). I don't think there is anormalize
option tosize
, but can't double check because I'm in the airX
and lower casey
?X
should be created ascancer_train.drop(columns='Class')
, which is more generalizableX_train
and do the same for the test data.X_tune
/y_tune
in grdisearch, which should just be the same_train
insteadconcat
to add the predictions? Can't we just create a copy of the new columns the regular way in the original dataframe (if it is OK to modify at this point)? Or usedf.assign(pred=df['predictions'])
to just print it without overwriting the variable (if we have taughtassign
before here.y_test
and the predictions created incancer_test_predictions
, so that students get a better grasp of what it is.confusion_matrix
when we are saying thatCnfusionMatrixDisiplay
is the recommend way?cv
param ofcross_validate
does not seem that helpful?func=
inagg
range
does somewhere or do we just start using it?param_grid_lots
is not a great name, something likelarge_param_grid
sounds better. same with the plots name.n_jobs=-1
?fit()
function of GridSearchCV instance.estimatethat"GridSearchCV.best_estimator_
instead of refitting it (which is redundant and potentially time consuming).in Python"