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 default option from param_range in validation curve #129

Merged
merged 6 commits into from Jun 9, 2019

Conversation

agramfort
Copy link
Collaborator

No description provided.

@@ -84,7 +98,11 @@ def validation_curve(epochs, y, param_name, param_range, cv=None):
param_name="thresh", param_range=param_range,
cv=cv, n_jobs=1, verbose=0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we're at it, perhaps let's expose the b_jobs param.

Copy link
Member

Choose a reason for hiding this comment

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

what's that? :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

probably the n_jobs parameter for the validation_curve function: Currently hardcoded to 1, see line 85(old)/99(new)

@jasmainak
Copy link
Member

@agramfort CIs are failing ... maybe because of your French keyboard

@jasmainak
Copy link
Member

This looks like it could go into 0.2 easily. @agramfort are you done here? If so, I can help rebase and merge it.

autoreject/autoreject.py Outdated Show resolved Hide resolved
doc/whats_new.rst Outdated Show resolved Hide resolved
@agramfort
Copy link
Collaborator Author

agramfort commented Jun 5, 2019 via email

@codecov-io
Copy link

codecov-io commented Jun 5, 2019

Codecov Report

Merging #129 into master will increase coverage by 0.65%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #129      +/-   ##
==========================================
+ Coverage   92.02%   92.67%   +0.65%     
==========================================
  Files          10       10              
  Lines        1178     1187       +9     
==========================================
+ Hits         1084     1100      +16     
+ Misses         94       87       -7

Co-authored-by: Mainak Jas <mainakjas@gmail.com>
Co-authored-by: Alexandre Gramfort <alexandre.gramfort@m4x.org>
@jasmainak
Copy link
Member

Rebased, @agramfort anything else?

The values of the parameter that will be evaluated.
If None, 15 values between the min and the max threshold
will be tested.
Copy link
Member

Choose a reason for hiding this comment

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

The problem doing this is that you will have np.inf as the error when the candidate threshold is the min threshold. Even if you do something like min threshold + epsilon (so that at least one epoch is retained), you have the same problem as for some folds it could end up being the min threshold

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

problem with None param or with a my custom grid?

@jasmainak
Copy link
Member

jasmainak commented Jun 8, 2019 via email

@agramfort
Copy link
Collaborator Author

agramfort commented Jun 8, 2019 via email

@jasmainak
Copy link
Member

added a test for you that fails

@agramfort
Copy link
Collaborator Author

if you get a loglik of -inf then it means the model is bad and to me it's not too much of a problem as you want to find the argmax of test_scores (so -inf will be ignored).

do I miss something?

@jasmainak
Copy link
Member

sure. But when you plot with np.inf then you have a ton of whitespace ...

@agramfort
Copy link
Collaborator Author

agramfort commented Jun 9, 2019 via email

@jasmainak
Copy link
Member

I can live with that

this is all I needed to know ;-)

@jasmainak jasmainak merged commit 7649deb into autoreject:master Jun 9, 2019
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.

None yet

5 participants