USGS-R / gsplot

plotting foundation for timeseries reporting
Other
6 stars 14 forks source link

tcl.minor default #460

Closed jordansread closed 7 years ago

jordansread commented 7 years ago

for #437

I ended up doing this in a way that isn't testable except for the result of the render...

This is because I don't want the fraction of par('tcl') to be calculated until it is relevant - i.e., when the side is being rendered, so it is stuck into the draw_axis w/ the arg = NA signifying to use this default. It is buried from the user, but the alternative solution that covers this and was discussed in #437 with the hierarchy is too complex for the value of this functionality. Additional thoughts welcome.

lindsayplatt commented 7 years ago

Since it's not testable except for rendering, did you build any of the vignettes? I imagine we would see a change because the tcl.minor default was 0.15 and should now be -0.25 if par('tcl') remains unchanged.

I think having NA carry through to the final gsplot object makes sense to me. People can just look in the documentation to figure out what the actual value should be since it could change if pars change.

jordansread commented 7 years ago

@lindsaycarr the tcl is in the default config already as 0.3. I will keep that as-is. Working on merge conflicts for the vignettes now and that is coming w/ the next commit.

jordansread commented 7 years ago

changes in the vignettes all seem to be due to the retina screen render. Note we don't have anything in the readme that calls out the tcl.minor, so we don't see changes there.

lindsayplatt commented 7 years ago

Doesn't it appear that the new fig changes are overplotting axes? Or is that just a visual trick?

jordansread commented 7 years ago

Hard to say if overplotting or retina. I didn't change anything related to append vs replace for axis, so not sure why this would overplot if it didn't before this commit.

jordansread commented 7 years ago

I added a change to the readme to force it to use the tcl.minor default @lindsaycarr

coveralls commented 7 years ago

Coverage Status

Changes Unknown when pulling 0aef4656d0f1e16e5afc97d61d2ed8cef0e19e13 on jread-usgs:tcl_minor_default into on USGS-R:master.