Skip to content
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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

dokato
Copy link
Contributor

@dokato dokato commented Jun 9, 2022

Details in #500

@codecov
Copy link

codecov bot commented Jun 9, 2022

Codecov Report

Merging #501 (55c92be) into master (49d578d) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            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              
Impacted Files Coverage Δ
R/neuronlistfh.R 48.94% <ø> (ø)
R/neuronlist.R 81.52% <100.00%> (+0.21%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 49d578d...55c92be. Read the comment docs.

Copy link
Collaborator

@jefferis jefferis left a 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
Copy link
Collaborator

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"

Copy link
Collaborator

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))
Copy link
Collaborator

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)
)
Copy link
Collaborator

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]

@dokato dokato changed the title added withMeta parameter to nlapply WIP: added withMeta parameter to nlapply Aug 18, 2022
@dokato
Copy link
Contributor Author

dokato commented Aug 18, 2022

Thanks @jefferis , maybe it requires more thinking on design indeed. The idea was to have access within the nlapply loop to the attributes of the neuron. So for example if I have a neuronlist nl with data.frame containing soma location or NULL, I can nlapply over nl and perform separate actions for each cases.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants