jaredlander / coefplot

Plotting model coefficients
Other
27 stars 19 forks source link

lwdOuter defaults to 0.5 instead of 0. #11

Closed xfim closed 4 years ago

xfim commented 8 years ago

With the previous default of lwdOuter at 0 no line was shown, although the definition of the outerCI was 2, and it did not make much sense to define the width of the outer interval but not have it plotted. With this new default a line half the width of the inner will be shown.

jaredlander commented 8 years ago

I like that but we have to make it OS specific. On Windows lwdOuter=0 works, but not on Mac. So the default argument should lwdOuter=if(Sys.info()["sysname"] == 'Windows') 0 else .5

xfim commented 8 years ago

Sorry @jaredlander . I'm afraid I don't follow. If you say "On Windows lwdOuter=0 works, but not on Mac". Then, ¿what is the problem of setting lwdOuter!=0? Doesn't work in some OS? From your proposal (the "if" statement) I infer that the problem is that Windows does not work with 0.5, but this is contradictory with your previous statement.

Sorry, but can you help me clarify?

Another issue would be that if the trouble is with half integers, we can make lwdOuter=1 and lwdInner=2. Would that be an acceptable solution?

jaredlander commented 8 years ago

Sorry, I didn't explain well. On Windows lwdOuter=0 and lwdInner=1 looks appropriate because the graphics device renders this well. So switching to lwdOuter=1 and lwdInner=2 universally would look good on Mac/Linux but not Windows. That's why we need the OS specific solution.

xfim commented 8 years ago

Let's try to clarify the situation.

This is the code that I have used to do the comparisons:

library(coefplot)

data(diamonds)
m <- lm(price ~ cut, data=diamonds)

coefplot(m)
coefplot(m, lwdInner=1, lwdOuter=0)
ggsave("c-inner1-outer0-win.png", width=8, height=4)

# The idea would be to do something like this, to show the two uncertainty bands by default
coefplot(m, lwdInner=2, lwdOuter=1)
ggsave("c-inner2-outer1-win.png", width=8, height=4)

# The current pull request does
coefplot(m, lwdInner=1, lwdOuter=0.5)
ggsave("c-inner1-outer05-win.png", width=8, height=4)

This is how the defaults look like in Linux: c-inner1-outer0 As you can see, there is only a single line. So currently in Linux only one line is displayed.

By contrast, in Windows the current defaults show two lines: c-inner1-outer0-win

If we use the pull request proposal lwdInner=1, lwdOuter=0.5, the Linux version displays two lines c-inner1-outer05 And the Windows version also displays two lines, correctly (to my understanding, although the thinnest one is thicker than the current default at lwdOuter=0). c-inner1-outer05-win

One option would be to generally increase the thickness of the lines by doing lwdInner=2, lwdOuter=1. I do not dislike the solution. In Linux would look like: c-inner2-outer1

And in Windows as follows: c-inner2-outer1-win

Finally, another option would be to increase lwdOuter to just 0.1, so that the Windows version is almost unaltered and the Linux version at least displays two lines, like: c-inner1-outer01

What would be your preferred option?

jaredlander commented 8 years ago

Hmm, you're right I think 0.5 will make it not too big on Windows but visible on Linux/Mac and avoid the if statement. I'm convinced.

But your pr isn't passing Travis-CI. Once that is fixed I'll merge. Could just be a version issue, but that is purely a guess.

Alternatively, I could just make the simple change. It's only a few functions.

xfim commented 8 years ago

Ok, perfect. The problem with the PR is the fact that I didn't run the documentation, so that the Rd files were not generated. Now it should be solved. Otherwise, it's only 6 lines of code in 3 files, so you can do it yourself. Thank you for everything.

xfim commented 5 years ago

Any news on this being merged into the main branch?

jaredlander commented 4 years ago

Just made an update which implements a variant of your request. On Windows lwdOuter remains 0, on other OSs lwdOuter is 0.5, all by default. Gave you credit in the commit, the NEWS.md and in the PR message. Thanks for bringing this up and sorry for waiting so long to implement. Will be on CRAN in the coming weeks.