-
Notifications
You must be signed in to change notification settings - Fork 29
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
WIP: added withMeta parameter to nlapply #501
base: master
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## master #501 +/- ##
==========================================
+ Coverage 75.73% 75.75% +0.01%
==========================================
Files 48 48
Lines 5906 5910 +4
==========================================
+ Hits 4473 4477 +4
Misses 1433 1433
Continue to review full report at Codecov.
|
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.
Thanks for this @dokato! I think I see the motivation but could you maybe explain quickly when you have needed this? I have added some points to fix as line comments.
@@ -388,6 +388,8 @@ as.data.frame.neuronlist<-function(x, row.names = names(x), optional = FALSE, .. | |||
#' error. The default value (\code{NA}) will result in nlapply stopping with | |||
#' an error message the moment there is an error. For other values, see | |||
#' details. | |||
#' @param withMeta whether or not to include the metadata from the neuronlist |
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 don't think this is 100% clear yet. I think you need to add something like "in the body of each object in the output list"
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.
Is the intention that the metadata dataframe should be removed from the output? It is. Maybe this should be signalled.
if (withMeta && ncol(X[,]) > 0) { | ||
meta_list <- split(X[,], seq(nrow(X[,]))) | ||
Xlist = mapply(append, X, meta_list, SIMPLIFY = FALSE) | ||
X = as.neuronlist(lapply(Xlist, as.neuron)) |
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.
This doesn't work if X contains dotprops rather than neuron objects. I think you need to set the class based on the class of each element of X before appending. I guess you have to use append
in case your metadata overwrites one of the existing fields in the elements of X? NB I have tried to avoid assuming that every element of a neuronlist has the same class.
out2 = nlapply(kcs13, function(x) x, withMeta = T) | ||
expect_true( | ||
length(attributes(out1[[2]])$names) < length(attributes(out2[[2]])$names) | ||
) |
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'm not sure this was the intention:
> out1
'neuronlist' containing 3 'dotprops' objects and 'data.frame' with 14 vars [75.9 kB]
> out2
'neuronlist' containing 3 'list' objects and 'data.frame' with 0 vars [94.5 kB]
Thanks @jefferis , maybe it requires more thinking on design indeed. The idea was to have access within the I know it's possible with subset, but then you need to stitch neuronlists together, which messes the order etc. I found out a couple of times that it'd be handy. |
Details in #500