stephenslab / susieR

R package for "sum of single effects" regression.
https://stephenslab.github.io/susieR
Other
176 stars 45 forks source link

susie_get_objective #58

Closed stephens999 closed 5 years ago

stephens999 commented 5 years ago

I think we should add this as an accessor function to the final elbo achieved

gaow commented 5 years ago

@stephens999 are you suggesting adding:

susie_get_objective = function(s) {
  s$elbo
}

?

stephens999 commented 5 years ago

no, only the last value, s$elbo[length(s$elbo)]

we could have an option to get the full vector.... but by default I'm just interested in the value obtained so we can easily compare fits from different initializations.

On Fri, Oct 19, 2018 at 10:46 AM gaow notifications@github.com wrote:

@stephens999 https://github.com/stephens999 are you suggesting adding:

susie_get_objective = function(s) { s$elbo }

?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/stephenslab/susieR/issues/58#issuecomment-431408280, or mute the thread https://github.com/notifications/unsubscribe-auth/ABt4xaz_zgM3jHtmLZ2QYO4LIDV35mCpks5umfPbgaJpZM4XwmHq .

stephens999 commented 5 years ago

it has been modified so that if it is non-decreasing then the whole thign is returnd with a warning. But the problem is that this non-decreasing check is quite delicate (can be very close to 0). And in programming this behaviour (switching to return whole thing when user did not ask for it) is disruptive.

I suggest: i) change check to some tolerance eg 1e-6 instead of 0. ii) if check fails emit warning, but do not change value of all.

gaow commented 5 years ago

@stephens999 Thanks for the suggestions -- both have been implemented.

zouyuxin commented 5 years ago

@gaow I have a susie fit with elbo at each iteration as -Inf -1941.380 -1914.195 -1914.195. The 3rd and the 4th elements are the same. elbo[3] == elbo[4] is TRUE. But susie_get_objective gives warning: Objective is not non-decreasing.

gaow commented 5 years ago

Sorry @zouyuxin it's an artifact of the patch from 3 days ago (see above). The patch below should have fixed it ...

 susie_get_objective <- function(res, all = FALSE, warning_tol = 1E-6) {
-  if (!all(diff(res$elbo) >= warning_tol)) {
+  if (!all(diff(res$elbo) >= (-1 * warning_tol))) {
     warning('Objective is not non-decreasing')
   }
stephens999 commented 5 years ago

Can we close this?