-
Notifications
You must be signed in to change notification settings - Fork 15
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
Implement Head and Tail methods #176
base: master
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## master #176 +/- ##
========================================
+ Coverage 89.77% 90% +0.23%
========================================
Files 102 104 +2
Lines 6151 6533 +382
========================================
+ Hits 5522 5880 +358
- Misses 629 653 +24
Continue to review full report at Codecov.
|
This is currently blocked due to overly strict validation on the csv export endpoint on the server. |
The ticket for the validation is: https://www.pivotaltracker.com/story/show/154086828 Remaining todos on the R side are:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few changes to the methods of dispatch.
I also added some tests that fail when using with an exclusion filter. This is another call to adding the ability in the backend to return arbitrary row (orders) https://www.pivotaltracker.com/story/show/151013797 Thought it's possible we can still work around the exclusion filter here with similar clever harmonizing magic like is here with filters.
#' @export | ||
setMethod("tail", "CrunchDataFrame", function (x, n=6L, ...) { | ||
return(tail(attr(x, "crunchDataset"))) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't look like these would respect the ordering of the CrunchDataFrame which is in the row.order
attribute.
I don't think it would be too difficult to add that, though we haven't been using that feature too much (it was designed to make merging onto simple feature style geographies possible, but I haven't had the time to push forward on that recently)
#' @aliases head tail | ||
NULL | ||
|
||
#' @rdname head-tail |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the head
/tail
methods that we are imitating are based on S3 classes, we should probably match those and define this (and similar for the following method definitions) as:
head.CrunchDataset <- function ...
Then remove the lines in AllGenerics.R that make the generics. This will prevent masking when the package is loaded.
It looks like this PR is blocked by some odd behaviour around datasets which have an exclusion. When you have a dataset with an exclusion, then try to pass a logical row filter, the API returns incorrect data. We think that this is because the row filter is being applied to the unexcluded data. Decision is to wait on this method until we implement https://www.pivotaltracker.com/n/projects/931610/stories/151013797 at which point we can refactor all of the |
If row filtering on the server doesn't respect the exclusion filter, that sounds like a bug that needs fixing (independent of this PR). |
Creates head and tail methods for Crunch Datasets, Dataframes, and Variables, this also implements numeric subsetting on dataset rows.