natverse / neuprintr

R client utilities for interacting with the neuPrint connectome analysis service
http://natverse.org/neuprintr
3 stars 3 forks source link

Change `id2json uniqueids` default in some functions? #39

Closed romainFr closed 4 years ago

romainFr commented 4 years ago

Or add a parameter in neuron_get_meta or neuprint_get_neuron_names, as those functions are often used to add columns to an existing data frame -- in which case we would want to use all bodyids, not unique values.

jefferis commented 4 years ago

Fair enough. I made that the default since most of the previous cases I could find were checking for unique bodyids, but as you say there are others that are not.

https://github.com/natverse/neuprintr/commit/90aca386847d1d0ff589021c37f6d7a11e4112aa

Perhaps it is better not to make this the default since it is probably more surprising to drop non-unique values than keep them.

jefferis commented 4 years ago

@alexanderbates most of the cases where there were there was stuff like:

jsonlite::toJSON(unique(as.numeric(unlist(bodyids))))

were from you I think. Is there a reason why you found it necessary to do this unique operation?

jefferis commented 4 years ago

Incidentally the better thing to do (and what I have generally done with catmaid) is to run the query with unique values and then map them back to the inputs. Easy for vectors, more fiddly for data.frames. Looks like this:

https://github.com/natverse/rcatmaid/blob/f92b3655124bc8327a4ad10a0d18bf1ef9ca2ce0/R/catmaid_metadata.R#L18-L25

catmaid_get_neuronnames<-function(skids, pid=1, conn=NULL, ...) {
  skids=catmaid_skids(skids, conn = conn, pid=pid)
  if(any(duplicated(skids))) {
    uskids=unique(skids)
    unames=catmaid_get_neuronnames(uskids, pid=pid, conn=conn, ...)
    res=unames[match(skids, uskids)]
    return(res)
  }
romainFr commented 4 years ago

It makes sense to use unique values for connectivity queries (adjacency matrices & co). We should probably just change the default for meta, name and roiInfo with an argument to switch to unique values?

jefferis commented 4 years ago

Thanks @romainFr. I will actually change the default as I feel it is more surprising, but explicitly request unique values in those connectivity queries as you say.

jefferis commented 4 years ago

closed by #42