search

dicook / nullabor

Tools for doing statistical inference using data plots
http://dicook.github.io/nullabor/
56 stars 10 forks source link

Eliminate a coercion to tibble #15

Closed jennybc closed 5 years ago

jennybc commented 6 years ago

I'm helping @krlmlr to assess the potential impact of changes in dev tibble. I selected this package as one of the experiments.

There is an example in this package that fails with dev tibble:

https://github.com/krlmlr/tibble/blob/f-revdep-2/revdep/new-problems.md#nullabor

This PR fixes that and would make this package work with current CRAN tibble and the coming tibble release.

I'm no expert in time series, so you should sanity check what I've done, but this seems like a good simplification. It doesn't seem necessary to coerce the object to a tibble and then to a vector. If you can eliminate coercing to a tibble, then we don't have to deal with the fact that the variable has no name.

jennybc commented 5 years ago

Just checking @dicook: did you mean to close this without merging it?

dicook commented 5 years ago

Yes, I did. I’ll make the change manually. Hopefully this is ok with you.

On 6 Mar 2019, at 11:39 am, Jennifer (Jenny) Bryan notifications@github.com wrote:

Just checking @dicook https://github.com/dicook: did you mean to close this without merging it?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dicook/nullabor/pull/15#issuecomment-469917807, or mute the thread https://github.com/notifications/unsubscribe-auth/AAPdB_WNDhM_2BKB7rwG6ozYzxJUGiYRks5vTw4xgaJpZM4YclMA.

Dianne Cook visnut@gmail.com

jennybc commented 5 years ago

Hopefully this is ok with you.

Sure.

If you're going to make this exact change, the "social coding" norm would be to accept the PR.

But if you've got a different fix in mind, then it makes sense to close and deal with it your preferred way.

dicook commented 5 years ago

That would assume I possess “social skills” %^)

I was bothered because I only just saw that PR, and realised that it had been created months ago. I need to get a bit of time to work on nullabor, and will handle it with other changes then.

On 7 Mar 2019, at 7:07 am, Jennifer (Jenny) Bryan notifications@github.com wrote:

Hopefully this is ok with you.

Sure.

If you're going to make this exact change, the "social coding" norm would be to accept the PR.

But if you've got a different fix in mind, then it makes sense to close and deal with it your preferred way.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dicook/nullabor/pull/15#issuecomment-470256979, or mute the thread https://github.com/notifications/unsubscribe-auth/AAPdByH5hy4vLNMs1XW1_wr-46UpuOy9ks5vUCAYgaJpZM4YclMA.

Dianne Cook visnut@gmail.com