ddalthorp / GenEst

R package development for a generalized mortality estimator
Other
6 stars 6 forks source link

restrict split refinement? #548

Closed juniperlsimonis closed 6 years ago

juniperlsimonis commented 6 years ago

it looks like the split calculation is able to proceed even if the split is refined to the extreme, as a ridiculous example, it works when I use "ID" as the split, which doesn't really make sense but is an extreme example.

the level of splitting can cause the figure to error (the margin size issue, so its within the plot code) in some instances, which I will limit at the GUI end of things (both from an input and output perspective), but @ddalthorp is this something that you think needs to get addressed in the command line of the split code as well?

ddalthorp commented 6 years ago

No. I don't think it makes sense to set an arbitrary level of allowable refinement on splits.

Obviously, there's a restriction on what kinds of graphs can be drawn in the GUI, but the statistics still work and summary tables do too.

juniperlsimonis commented 6 years ago

i've patched this issue with v0.2.12 #554

It's actually a limit within the base plotting function for splits, not within the GUI in order to handle the errors being thrown, i've introduced a catch in plot.splitFull that throws an error if there are over 100 levels of the split vertically within the output, and suggests transposing (which can be plotted with 100s on the horizontal axis) i'm not sure what the right cut-off is here, and 100 is likely far too high still, so it may need to be adjusted in the future

ddalthorp commented 6 years ago

what about putting "plot" in a tryCatch? Then you don't need to worry about where to draw the line.

juniperlsimonis commented 6 years ago

believe me i have tried wrapping things in tryCatch at every stage of the plotting hierarchy and it doesn't work given how you've set up the plot calls

ddalthorp commented 6 years ago

what about this?

if (!is.null(try(plot(...))) stop(...)

ddalthorp commented 6 years ago

do you have a data set that throw the error?

juniperlsimonis commented 6 years ago

from the PR

unfortunately, given the way the plot code is for the splits, that actually still throws an error about the figure margins (thrown by plot.new()) before the error is thrown by plot.splitsFull(), and which isn't collected by the try functions at that level because of the scoping of plot objects. that means it doesn't address the issue that we're trying to avoid here.

juniperlsimonis commented 6 years ago

most (all?) of them have this problem when you split on ID the mock data is a good one to play with since there are >400 carcasses, but I'm working in the wind_RP one right now too

ddalthorp commented 6 years ago

OK.

Thanks