lukesonnet / KRLS

R package for machine learning technique to fit flexible, interpretable functional forms for continuous and binary outcomes.
Other
21 stars 12 forks source link

Add warning for solution near lambdainterval endpoints #5

Closed lukesonnet closed 7 years ago

lukesonnet commented 7 years ago

great. I wonder if it's worth throwing an error (or at least warning) if the optimization hits the edge. e.g. f <- function (x) (x - .5)^2

tryoptimize = function(f, interval){ xmin <- optimize(f, interval = interval) tooclose=abs(xmin$minimum-lambdainterval)<.Machine$double.eps^.25 if(tooclose[1]) stop("Too close to lower bound of lambda interval; please decrease lower bound.") if(tooclose[2]) stop("Too close to upper bound of lambda interval; please raise upper bound.") }

tryoptimize(f, interval=c(0,1)) tryoptimize(f, interval=c(1,2))

lukesonnet commented 7 years ago

I've implemented this like so:

  fit.lambda <- optimize(lambdab.fn, interval=log(control$lambdainterval),
                      Kdat = Kdat, y=y, folds=length(control$chunks),
                      chunks = control$chunks, ctrl = control,
                      b = b,
                      vcov = FALSE)

  # Test for closeness in log scale or else this test is too sensitive in smaller regions of lambda
  solution_on_interval <- abs(fit.lambda$minimum - log(control$lambdainterval)) < .Machine$double.eps^0.25

  if (solution_on_interval[1]) 
    stop("Lambda solution too close to lower bound of lambdainterval; please decrease lower bound.")

  if (solution_on_interval[2]) 
    stop("Lambda solution too close to upper bound of lambdainterval; please increase upper bound.")

It seems to work fine. You'll notice I check if the solution gets close to the log of the interval, which I think makes sense because then it isn't too sensitive to solutions that are in the smaller end (1e-5 and such).

I'm closing this unless errors come up in the simulations.

lukesonnet commented 7 years ago

On second thought, this doesn't work great. Using the log scale causes too many errors on the high end and using the regular scale causes too many errors on the low end. Perhaps this should just be a warning? Because finishing close to 10^-12 isn't really too low, its just that it thinks lambda should basically be 0. We should think about ways to be smarter about this interval.

Currently, it checks if it is within 1e-13 of the lower end and within 1e-5 within the upper end, allowing for us to get down close to 0 but only somewhat close to the upper end.

chadhazlett commented 7 years ago

I forget how it worked out but think it needs to be a wider margin than .Machine$double.eps^0.25, more like 2*.Machine$double.eps^.5

If we're doing the search in the loglambda space then my sense is that we should be checking whether loglambda is within 2*.Machine$double.eps^.5 of the edge. Also that prevents it from being something on the order of 10^-7, and we're willing to tolerate bigger mistakes at the top than at the bottom so I think the log scale is the right one.

On Sat, Jul 29, 2017 at 9:35 AM, Luke Sonnet notifications@github.com wrote:

On second thought, this doesn't work great. Using the log scale causes too many errors on the high end and using the regular scale causes too many errors on the low end. Perhaps this should just be a warning? Because finishing close to 10^-12 isn't really too low, its just that it thinks lambda should basically be 0. We should think about ways to be smarter about this interval.

Currently, it checks if it is within 1e-13 of the lower end and within 1e-5 within the upper end, allowing for us to get down close to 0 but only somewhat close to the upper end.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/lukesonnet/KRLS/issues/5#issuecomment-318831504, or mute the thread https://github.com/notifications/unsubscribe-auth/AFVCyLgZ8gywKVfuQCPPc_nWd4h7Tsunks5sSzUSgaJpZM4OnRTu .

lukesonnet commented 7 years ago

I've followed your suggestion and gone back to the log scale, just with the larger tolerance you recommend. This new approach hasn't failed in a bunch of simulations or in a tiny sample so I think it's alright for now. I'm a little worried about this being an error rather than a warning. I'm going to leave this open as we keep trying it out in different use cases.

chadhazlett commented 7 years ago

Maybe warning would be better actually

On Jul 29, 2017 12:06, "Luke Sonnet" notifications@github.com wrote:

I've followed your suggestion and gone back to the log scale, just with the larger tolerance you recommend. This new approach hasn't failed in a bunch of simulations or in a tiny sample so I think it's alright for now. I'm a little worried about this being an error rather than a warning. I'm going to leave this open as we keep trying it out in different use cases.

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/lukesonnet/KRLS/issues/5#issuecomment-318840665, or mute the thread https://github.com/notifications/unsubscribe-auth/AFVCyOWT2blnctQgZ-dsn3ksu4zgfXXfks5sS1h1gaJpZM4OnRTu .

lukesonnet commented 7 years ago

I think warning makes more sense, especially give that I've widened the default a bit. I'll make that change now. I've also added a 'warn' argument where you can set the global warnings argument. I do this because I want to default our software to 1, which means print warnings as they occur, and power users can change that if they want.

chadhazlett commented 7 years ago

Great

On Jul 29, 2017 12:27, "Luke Sonnet" notifications@github.com wrote:

I think warning makes more sense, especially give that I've widened the default a bit. I'll make that change now. I've also added a 'warn' argument where you can set the global warnings argument. I do this because I want to default our software to 1, which means print warnings as they occur, and power users can change that if they want.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/lukesonnet/KRLS/issues/5#issuecomment-318841942, or mute the thread https://github.com/notifications/unsubscribe-auth/AFVCyDswaTFMl4gnFoTiTRvPhT2-JLXmks5sS11mgaJpZM4OnRTu .

lukesonnet commented 7 years ago

This seems to be working for now. But I also haven't run into edge cases so its hard to know if we are missing some where we should warn. Closing for now.