ggalluvial:::data_to_xspline() throws an error when data arg has less than 2 rows.

[stevegbrooks]

Description of the issue

ggalluvial:::data_to_xspline() throws an error when data arg has less than 2 rows.

Reproducible example

Using the example on the "Tooltips" vignette, if you slice the data arg to only have 1 row, then it doesnt work.

E.g., ggalluvial:::data_to_xspline(groups_to_draw[[1]][1,], knot.prop = T) will throw the error "Error in rep(4, nrow(data) - 2) : invalid 'times' argument"

[corybrunson]

@stevegbrooks good call! Thanks for bringing this up.

The reason is that data_to_xspline() was originally only used internally, and as such was not designed for flexibility. (It was only used by GeomAlluvium$draw_group() in cases where data has more than one row.) Edit: In the meantime, you'll just need to handle the case that data has only one row manually, e.g. using if and then.

It might be appropriate to export the function to users, since inviting users to use unexported objects via ::: can cause myriad problems. @qdread what do you think? We could make this change in the next release.

[stevegbrooks]

ah makes sense. Yeah I just implemented tooltips for an alluvial plot that I built at work, and had to write a hackey if statement that does an rbind() to double up the single row inside this lapply from the vignette below:

group_xsplines <- lapply(groups_to_draw, ggalluvial:::data_to_xspline, knot.prop = TRUE)

Seems to work ok - but I'll look forward to taking that out of my code once 1.0 is released :)

[qdread]

Yes, we should probably export the function. I wonder if there is a good way to modify data_to_xspline so that it produces sensible output with one row of input?

[corybrunson]

Looking at the code, i think what makes the most sense is to embed an if–else within both curve generators, i.e. data_to_xspline() and data_to_unit_curve(). Or maybe just export a single function like data_to_alluvium() that takes "spline" as the default value for the curve_type parameter. Since there would be one fewer intermediary between the ggproto function and the user-level function, this would also make tooltips easier, right?

I can do this in a branch once i come back to the Shiny vignette issues, since those are still in my court.

[qdread]

Yes I was thinking along those lines but didn't know if it would be too unwieldy. Let me know if there's some way I can help!

[corybrunson]

@qdread it will still be a while before i can get to it. If you want to give this a try in a new branch, please do! i'll be glad to test it out.

[qdread]

Hey I just took an initial stab at this. Haven't tested yet. I combined the two curve generating functions into one so that the old code in data_to_xspline is executed if curve_type %in% c('spline','xspline') and the data has more than two rows. If not, the old code in data_to_unit_curve() is executed, temporarily converting curve_type to "linear" if it is trying to draw a spline with only 2 rows in data. https://github.com/qdread/ggalluvial/commit/9d028a04b2e9a2140298e62a32aff2422f5c503b

[corybrunson]

@qdread thank you so much. I will take a close look next weekend.

[qdread]

Hi again Cory, I made another change to the code in my branch https://github.com/qdread/ggalluvial/commit/49a6eb8cb141b4a2a918ec4579d3f6e7da080df3. I moved all the logic inside the data_to_alluvium() function including testing whether the data has a single row. That way, the function can be used in the vignette -- maybe you can export the function if you think it is a good idea. It will require some changes to the vignette because the default arguments to data_to_alluvium() are now different. I may try to modify that as well.

I did run all this through your unit tests and it passed but probably that isn't the best way to test this particular change.

[qdread]

Hmmm, maybe I was a bit too hasty in pushing this stuff. I noticed some issues. Trying to fix them and will get back to you. I will try to clean it up and make a pr.

[corybrunson]

@qdread i had so many experiences like this early on! Thanks for flagging that.