Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Rework simplification #909
Rework simplification #909
Changes from 20 commits
bcc5415
1ead93b
a927496
be3fe11
17bffab
8d63020
02ea731
71ba50c
20387d0
7805591
5d6d874
d9c76d3
1980e24
eb85559
5be021d
5940cc2
675a8a8
8b32bfa
a4f864d
822d714
5254896
c8b4bc9
e4311be
80fa90c
41b2039
a7a41c8
4577473
5319949
fcb92d6
2397f21
471f340
b16071a
da1f7c9
0590ccf
99c925e
808568b
2c5866a
78938f7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
No
@description
? I feel like these details would probably make a decent description insteadThere 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.
Oops. No idea why I did that.
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.
Maybe do some validation of
strict
?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.
How do we feel about
list_simplify(x, strict = FALSE)
when the elements ofx
aren't vectors? Like:I feel like this should still error, because purrr functions should only work on vector types?
I feel like
strict
should only be for:i.e. this scalar object issue is out of scope and would still be an error
If you agree, then I'd argue that the current implementation here will be very slow (
vec_is()
is quite slow) and you could use this instead: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.
Even if you want to support scalar types, it might be worth using the approach above and wrapping it in
tryCatch()
that returnsFALSE
on error, as it will still probably be an order of magnitude or more faster thanvec_is()
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 actually isn't clear to me that this is supposed to be a character vector.
It seems like it is supposed to be a named vector where the names get used as the output names, like
c(x = 1, a = 2)
or something like that.Is there any reason not to call it
names
like intranspose()
?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.
Oops forgot to update these docs. Hopefully it's more obvious why it's called
template
and not names when you realise it can also take positions.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.
I think it is very useful to mention that
template
can be an integer vector to do positional transposition.Like, I didn't know that
template
could ignore names like this until i got to this exampleThere 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.
You say
by output column
a few times, but there aren't any columns in this operation right?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.
Random comment:
I tried this somewhere along the way while exploring this and this error didn't make much sense to me
Also it looks like a
call
needs to be passed throughThere 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.
Now: Length of
default
(1) andtemplate
(2) must be the same when transposing by position.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.
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 also works if
.simplify
isNA
right?