paternogbc / sensiPhy

R package to perform sensitivity analysis for comparative methods
http://onlinelibrary.wiley.com/doi/10.1111/2041-210X.12990/full
GNU General Public License v2.0
12 stars 4 forks source link

Potential error with error-handling in clade_phylm? #165

Closed gijsbertwerner closed 6 years ago

gijsbertwerner commented 7 years ago

Was just working on xx_Discrete functions again, when I noticed that in clade_phylm (and perhaps also in other clade functions, but haven't checked), there may be an error in how errors are handled (isn't that ironic!).

For instance in influ_phylm we get this call, which saves errors to a separate object:
if(isTRUE(class(mod)=="try-error")) { error <- i names(error) <- rownames(full.data$data)[i] errors <- c(errors,error) next }

But, I don't see any of this in clade_phylm. If I understand correctly this means that we are actually testing for errors, but not saving/flagging them if they are there?

paternogbc commented 7 years ago

Hey @gijsbertwerner ,

you are correct. I am not using the try approach anymore. Decided that because the phylolm functions dont seem to produce any errors (as far as I managed to test).

We started using the TRY approach before we changed the package to phylolm. Basically the 'gls' function (which we used before to run the models) was giving many errors during loops (the FALSE CONVERGENCE ERROR) usually related to lambda estimation by maximum likelihood falling out of bounds. Them we developed this solution, which basically skip the iteration in the loop and also keep record of which simulation actually failed (very important to know for the influ_xxx funtions).

So, it means that we are not testing nor flagging any errors in clade_xxx. However, if an error occurs within our function it will stop the run entirely (that was why we introduced this approach, so when errors occurred we could simply skip to the next iteration). Do you think we should put it back?

gijsbertwerner commented 6 years ago

No, I think this is fine, it all makes sense to me now. Just wanted to make sure that this was not an error.