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

Evaluate against multiple measures #104

Merged
merged 4 commits into from
Mar 20, 2019
Merged

Conversation

ayush1999
Copy link
Contributor

Initial fix for : #98

@ablaom
Copy link
Member

ablaom commented Mar 18, 2019

A note: TunedModel in src/tuning.jl makes a call to fit Resampler objects. It must now call using measures= instead of measure but note that in this case it always calls with a single measure. Do not change the keyword argument of TunedModel to measures, keep it as measure. When we tune, we must fix just one measure.

@ayush1999
Copy link
Contributor Author

@ablaom Sure, it'd be easier to just change the constructor for Resampler to use the measures keyword argument instead of measure, right?

@ablaom
Copy link
Member

ablaom commented Mar 18, 2019

Yes, the Resample constructor should have measures instead of measure - just a name change. However, this will break fit!(::EitherTunedModel{Grid}, which constructs a Resampler object (from src/tuning.jl):

    resampler = Resampler(model=clone,
                          resampling=tuned_model.resampling,
                          measure=measure,
                          operation=tuned_model.operation)

I think all that is necessary to fix this is to change the third line to measures=measure.
Clear?

@ayush1999
Copy link
Contributor Author

Got it. Will push the changes.

@ayush1999
Copy link
Contributor Author

@ablaom Please review.

@ablaom
Copy link
Member

ablaom commented Mar 18, 2019

Great, thanks for that.

  • There is a minor bug on line 90 of resampling.jl (I don't understand why travis does not detect it; maybe because it is in logging??). The line should read:
        "measures=$_measures \n"*
  • And a similar problem on line 137.

  • Also the reporting of cv results is not exactly as specified. For easy aggregation later, we want a named tuple of vectors, not a vector of named tuples. For example, for this code:

x1 = ones(10)
x2 = ones(10)
X = DataFrame(x1=x1, x2=x2)
y = [1.0, 1.0, 2.0, 2.0, 1.0, 1.0, 2.0, 2.0, 1.0, 1.0]

cv=CV(nfolds=5)
model = ConstantRegressor()
mach = machine(model, X, y)
evaluate!(mach, resampling=cv, measures=[rms, rmslp1])

we get

 (MLJ.rms = 0.5, MLJ.rmslp1 = 0.22314355131420982)
 (MLJ.rms = 0.75, MLJ.rmslp1 = 0.287682072451781) 
 (MLJ.rms = 0.5, MLJ.rmslp1 = 0.22314355131420982)
 (MLJ.rms = 0.75, MLJ.rmslp1 = 0.287682072451781) 
 (MLJ.rms = 0.5, MLJ.rmslp1 = 0.22314355131420982)

But we want

(MLJ.rms=[0.5, 0.75, 0.5, 0.75, 0.5], MLJ.rmslp1=[0.223, 0.287, ...., ])
  • For testing the multi-measure case, please add the following after line 23 of test/resampling.jl:
result = evaluate!(mach, resampling=holdout, measures=[rms, rmslp1])
@test result isa NamedTuple

And, after the current line 37, add

result = evaluate!(mach, resampling=cv, measures=[rms, rmslp1])
@test result isa NamedTuple

@ablaom
Copy link
Member

ablaom commented Mar 20, 2019

Ah, my mistake. My new test mucks up the consequent test. How about you just comment out the new test lines and I will fix this later?

@ablaom
Copy link
Member

ablaom commented Mar 20, 2019

Looks like your error this time. You have an array where a number is expected. Let me know if you want me to have a look at it.

@ayush1999
Copy link
Contributor Author

Ah, my bad. I forgot to test cases when single measure is used. Fixed it this time.

@ablaom ablaom merged commit 1bbfac1 into JuliaAI:master Mar 20, 2019
@ablaom
Copy link
Member

ablaom commented Mar 20, 2019

Thanks!

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