Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Free scale facet axes #253

Merged
merged 14 commits into from
Nov 19, 2024
Merged

Free scale facet axes #253

merged 14 commits into from
Nov 19, 2024

Conversation

grantmcdermott
Copy link
Owner

Enables a facet.args(free = TRUE) argument.

Closes #189.

Getting this to work properly requires some trickery behind the scenes to capture and reset par(usr) for each individual facet correctly. As explained in #90 (comment):

We have to manually reset par("usr") when drawing the plot elements of each individual facet at the end of tinyplot(). If we don't reset par("usr"), then each facet's extent is determined by the final facet plot region that was drawn higher up in the code logic. (Remember: We have to draw all of the plot regions before the drawing all the plot elements, otherwise we can't get facets to play nice with grouping.)

But everything seems to be working correctly AFAICT.

MWEs

1.a) Points with fixed facets.

plt(
  mpg ~ wt, data = mtcars, pch = 19,
  facet = ~ vs + am, grid = TRUE
)

1.b) Points with free facets.

plt(
  mpg ~ wt, data = mtcars, pch = 19,
  facet = ~ vs + am, grid = TRUE,
  facet.args = list(free = TRUE),
)

2.a) Histogram with fixed facets.

data("penguins", package = "palmerpenguins")

plt(
  ~flipper_length_mm | species, data = penguins, facet = "by",
  type = "hist",
  grid = TRUE, legend = FALSE
)

2.b) Histogram with free facets.

plt(
  ~flipper_length_mm | species, data = penguins, facet = "by",
  type = "hist",
  grid = TRUE, legend = FALSE,
  facet.args = list(free = TRUE)
)

Merge branch 'main' into facet_free

# Conflicts:
#	R/facet.R
- works with tinyAxis
- assign and get facet usr pars through an env var (list)
@vincentarelbundock
Copy link
Collaborator

Oh this is great!

FYI, ggplot2 has

scale = "free"
scale = "free_x"
scale = "free_y"

I've personally used the one-free-axis a lot, so if it's not too hard to implement I feel this would be important.

@grantmcdermott
Copy link
Owner Author

grantmcdermott commented Nov 13, 2024

Oh this is great!

FYI, ggplot2 has

scale = "free" scale = "free_x" scale = "free_y"

I've personally used the one-free-axis a lot, so if it's not too hard to implement I feel this would be important.

Yeah, agree having separate support for freely varying x and y axes would be nice. I'll take a crack at it this evening.

The other thing I haven't been able to resolve properly is free scaling for logged axes. This is more complicated than it first seems because of some internal magic that the graphics coordinates system does behind the scene. I might just add a warning if logged axes are detected and revert to face.args(free = FALSE) if I can't figure it out.

@grantmcdermott
Copy link
Owner Author

Note to self (and potential future implementation): I've disabled support for free facet scales if the logged axes are detected. I just can't quite crack the implementation and don't have time to further pursue this right now. We can investigate at some future date.

The tricky thing is that (re)setting par(usr) manually breaks some of the internal accounting that graphics uses behind the scenes to draw the axis. Some code that I've tried (incl. debugging with axTicks()) just after setting xfree and yfree here:

if (par("xlog")) {
  xfree = log(xfree)
  xaxpf = par("xaxp")
  xaxpf[3] = -abs(xaxpf[3])
  par(xaxp = xaxpf)
}
if (par("ylog")) {
  yfree = log(yfree)
  yaxpf = par("yaxp")
  yaxpf[3] = -abs(yaxpf[3])
  par(yaxp = yaxpf)
}

@vincentarelbundock
Copy link
Collaborator

@grantmcdermott Can you put this one on hold? I need to completely refactor the tpar() and axis drawing things to make themes work, so this is going to be a big conflict.

@vincentarelbundock
Copy link
Collaborator

To give you an idea, we can drop tinyAxis() algother, get a ton more flexibility and transparency, and simplify the code as follows:

      # axes, frame.plot and grid
      if (isTRUE(axes)) {
        args_x = list(x,
          side = xside,
          cex = get_tpar(c("cex.xaxs", "cex.axis"), 0.8),
          col = get_tpar(c("col.xaxs", "col.axis"), "grey90"),
          lwd = get_tpar(c("lwd.xaxs", "lwd.axis"), 0.5)
        )
        args_y = list(y,
          side = yside,
          cex = get_tpar(c("cex.yaxs", "cex.axis"), 0.8),
          col = get_tpar(c("col.yaxs", "col.axis"), "grey90"),
          lwd = get_tpar(c("lwd.yaxs", "lwd.axis"), 0.5)
        )
        type_range_x = type %in% c("pointrange", "errorbar", "ribbon", "boxplot", "p") && !is.null(xlabs)
        type_range_y = isTRUE(flip) && type %in% c("pointrange", "errorbar", "ribbon", "boxplot", "p") && !is.null(ylabs)
        if (type_range_x) {
          args_x = modifyList(args_x, list(at = xlabs, labels = names(xlabs)))
        }
        if (type_range_y) {
          args_y = modifyList(args_y, list(at = ylabs, labels = names(ylabs)))
        }
        if (isTRUE(frame.plot)) {
          # if plot frame is true then print axes per normal...
          do.call(Axis, args_x)
          do.call(Axis, args_y)
        } else {
          # ... else only print the "outside" axes.
          if (ii %in% oxaxis) do.call(Axis, args_x)
          if (ii %in% oyaxis) do.call(Axis, args_y)
        }
      }

@grantmcdermott
Copy link
Owner Author

@grantmcdermott Can you put this one on hold? I need to completely refactor the tpar() and axis drawing things to make themes work, so this is going to be a big conflict.

Sure, I can hold off. Currently knocking my head against a wall trying to make separate "x" and "y" free scaling work. I keep running into a new bug each time I fix an old one, so I'm happy to take an enforced break.

You may want to check with @zeileis, though since he was the original implementer of tinyAxis. IIRC it was necessary to support functionality like axes = "l" and also some spineplot features. But if the tests are passing, then that bodes well...

@grantmcdermott
Copy link
Owner Author

(Also: thanks for the HU about the potential conflict.)

@vincentarelbundock
Copy link
Collaborator

Unfortunately, I think none of the tests will pass because the main location and the axes are sliiiiightly different. I'll have to check each snapshot manually and generate new ones.

I'll put spineplot on the list of TODOs to make sure it is still possible.

@zeileis
Copy link
Collaborator

zeileis commented Nov 19, 2024

Just as a quick comment, not sure that it is really relevant/helpful here: I mainly implemented tinyAxis() in order to have a function like Axis() but with our newly-introduced vocabulary for axes/xaxt/yaxt. Originally, it was used only in draw_facet_window() but in different parts of that function with different settings. However, recently I also used it in the PR for type_ridge().

In type_spineplot() it was not possible to use tinyAxis() directly so this has its own unexported spine_axis() function that uses the same type vocabulary but is geared towards specific spineplot labeling.

@vincentarelbundock vincentarelbundock mentioned this pull request Nov 19, 2024
3 tasks
@vincentarelbundock
Copy link
Collaborator

OK, that makes total sense. Thanks for clarifying. tinyAxis() does sound very useful.

@grantmcdermott
Copy link
Owner Author

grantmcdermott commented Nov 19, 2024

@vincentarelbundock I know we said pause work on this PR above, but now I'm wondering whether we shouldn't just merge as-is (after I move it out of draft)?

While this doesn't give you separate x and y axis scaling, right now this PR imposes only a small change on the existing codebase. I think it will be easier to integrate these into your themes PR rather than the other way around.

We can revisit separate free x and y scaling after themes.

Agree?

@vincentarelbundock
Copy link
Collaborator

Yep, sounds good.

I'm having problems with themes and have a crazy week, so it'll take a while.

FYI, hooks for themes are difficult, because we often need to set a parameter that is used at the very start of tinyplot(), whereas the hook only gets activate (and themeing parameters only get set) when the device is initiated. I suspect this is why basetheme doesn't play well with many tinyplot elements.

@grantmcdermott
Copy link
Owner Author

All good and really appreciate you taking themes on. I think we should make themes the big feature for the next CRAN release cycle (i.e., after this one). So the timing should work out nicely.

@vincentarelbundock
Copy link
Collaborator

Or wait a couple weeks and make it a mega release with the and types. That would make a big splash, as they are the final two things that, for me, were needed to make this a daily driver.

@grantmcdermott
Copy link
Owner Author

Or wait a couple weeks and make it a mega release with the and types. That would make a big splash, as they are the final two things that, for me, were needed to make this a daily driver.

I definitely get the impulse. But my motivation here is partly strategic. I want to make sure that the new type system doesn't trigger any CRAN issues and avoid complicating this with a simultaneous themes/tpar refactor.

Let me open a separate issue where we can discuss this, though. (After I get the kids too school.)

@grantmcdermott grantmcdermott marked this pull request as ready for review November 19, 2024 18:11
@grantmcdermott grantmcdermott merged commit b1658ca into main Nov 19, 2024
3 checks passed
@grantmcdermott grantmcdermott deleted the facet_free branch November 19, 2024 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

facet free y and x scales
3 participants