search

ropensci / skimr

A frictionless, pipeable approach to dealing with summary statistics
https://docs.ropensci.org/skimr
1.1k stars 79 forks source link

Put skimmers in order when using focus(). #670

Closed elinw closed 2 years ago

elinw commented 2 years ago

This addresses #668

michaelquinn32 commented 2 years ago

Hi Elin!

Thanks for taking this on.

I believe that a lot of this logic already exists in reconcile_skimmers() https://github.com/ropensci/skimr/blob/97d0fa9f26c8e98912ba4079c44210f7bd88f382/R/reshape.R#L54

And the functionality is similar too. The point of that function is to make sure that columns and attributes are aligned. Is there a way to get everything working with that function?

Thanks!

elinw commented 2 years ago

@michaelquinn32 I don't think it's working the way it is called because of the way the print uses the skimmers_used that are copied directly from the original skim object. I tried to walk my way through the code and the wrong list of skimmers happens here https://github.com/ropensci/skimr/pull/670/files#diff-59ef9e95ee370d129669b5575809d3e584f61eab2dba02455ec36c4e95d7ab42R185

reconcile _skimmers() Also uses the skim object attributes https://github.com/ropensci/skimr/blob/840b6237597e3da7c8970a6581086ef73c561678/R/reshape.R#L56

So conceivably your could modify reconcile_skimmers() but then you'd have to pass through the selected columns. Meaning what I'm doing here to update the skim attribute is actually allowing reconcile_skimmers() to work correctly with focus().

elinw commented 2 years ago

Any thoughts?

elinw commented 2 years ago

@michaelquinn32 Any progress on this? We should probably be thinking about a new release soon.

michaelquinn32 commented 2 years ago

My PR is failing because of SF installation issues.

Otherwise, PTAL.

elinw commented 2 years ago

Ugh about sf, that's just supposed to be suggest ... maybe we should just stop with it. I think it's changed a lot anyway.

I have my very special StackOverflow question about installing GDAL.

michaelquinn32 commented 2 years ago

We could. I don't believe it to be a big deal either way. The reason sf failed is that a new version was released to cran, and we didn't have binaries.

elinw commented 2 years ago

Okay so I'm for merging, but I also think ... let's use a different example in the vignette. It is simple to write a skim.lm method (that skims the lmresults$model) so I can do that as the example (and also put in skimExtras. Thoughts? @michaelquinn32

michaelquinn32 commented 2 years ago

Thanks!

Merging now. I opened #675 to handle updating the vignette.