natverse / nat.nblast

R package implementing the NBLAST neuron search algorithm, as an add-on for the NeuroAnatomy Toolbox (nat) R package.
http://natverse.org/nat.nblast/
16 stars 6 forks source link

add plot3d.nblastres function and give nblast option to return per segment scores #29

Closed jefferis closed 8 years ago

jefferis commented 8 years ago

In order to satisfy one of the reviewer comments we should add a new function / example that shows which points are being matched for a pair of neurons and colours one of the neurons by the quality of the match. One way to do this would be allow the nblast function to return per segment scores (perhaps wrapping them in an object with a class like nblastres. A corresponding plot3d method could then be used to make a plot with sensible defaults.

Alternatively, a lower tech version would be to include an example in the nblast docs.

Collecting per segment results could be done by playing with the NNDistFun argument (which gets passed down to WeightedNNBasedLinesetMatching.default.

jdmanton commented 8 years ago

Plot-wise, what differences from the show_similarity output do you want? Do you want to colour the entire neuron in one uniform colour, given its match quality?

jefferis commented 8 years ago

I think I was imagining that this would be doing what show_similarity is now doing.

What do you see as the distinct use cases?

I did recently make a simple wrapper function for nblast searches again fly circuit data where there were then plot and summary methods for the results object. This was quite useful but not sure if we can make a general version.

Will try to find code.

G.

Sent from my iPhone

On 1 Apr 2016, at 09:32, James Manton notifications@github.com wrote:

Plot-wise, what differences from the show_similarity output do you want? Do you want to colour the entire neuron in one uniform colour, given its match quality?

— You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub

jefferis commented 8 years ago

@ajdm I think it would be good to add an example based on the kcs20 dataset to show_similarity and then to consider this issue closes at least for the purposes of the paper.

jdmanton commented 8 years ago

Ah, I wrote a response to the 'use case' question, but it looks like I never pressed the comment button. I thought that the main use cases were:

I'll add an example for kcs20, but none of these use cases beyond the first seem like they'll be demonstrated with that.

jefferis commented 8 years ago

Kcs20 gamma (unbranched) vs ab will sort of do 1 & 4. G.

Sent from my iPhone

On 26 Apr 2016, at 12:47, James Manton notifications@github.com wrote:

Ah, I wrote a response to the 'use case' question, but it looks like I never pressed the comment button. I thought that the main use cases were:

highlight where similar neurons are most (dis)similar, mirror a neuron and then use show_similarity on the unflipped and flipped version to inspect its bilateral symmetry, take a GAL4 line trace and a single-cell trace and compare them, use a neuron as a 'tract examplar' and see if other neurons are similar just through the tract, or if they have similar arborisations as well. I'll add an example for kcs20, but none of these use cases beyond the first seem like they'll be demonstrated with that.

— You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub

jdmanton commented 8 years ago

An example (perhaps not a particularly great one) using gamma and alpha-beta neurons added in 917a6e88e8fc3df756510e59fd109865d8f984d1.

jefferis commented 8 years ago

I'm getting this:

 library(nat.nblast)
> 
> library(nat)
> 
> # Pull out gamma and alpha-beta neurons
> gamma_neurons <- kcs20[kcs20[,]$type=='gamma']
> ab_neurons <- kcs20[kcs20[,]$type=='ab']
> 
> clear3d()
> show_similarity(ab_neurons[[1]], ab_neurons[[2]])
 Show Traceback

 Rerun with Debug
 Error in get(getOption("nat.nblast.defaultsmat")) : 
  invalid first argument 

because option("nat.nblast.defaultsmat") is NULL. Any idea why this is coming up?

jdmanton commented 8 years ago

Ah, I didn't add the change where I uncommented L5 in zzz.R. Then again, looking at the git blame log for that it looks like I should just change it to tolerate NULL, no?

jdmanton commented 8 years ago

it looks like I should just change it to tolerate NULL, no?

Done in 6b2e032ad5249a52648253309afbd7a378d2cb6c. I can easily reverse the commit if it should do something else instead.

jefferis commented 8 years ago

I think that is fine, although maybe we should just make like easier for ourselves and set a default smat as you did on testing. More importantly though are you sure the colours are working as expected.

When I do this:

# nb I think these next 2 lines may be better style for interactive use than the current version
gamma_neurons <- subset(kcs20, type=='gamma')
ab_neurons <- subset(kcs20, type=='ab')

clear3d()
show_similarity(ab_neurons[[1]], gamma_neurons[[3]])

I get this

show_similarity_test

Although the calyx looks red, which I assume to be bad, the dorsal tip of the alpha lobe doesn't look that bad but seems an equally long way from anything to me.

jdmanton commented 8 years ago

I did wonder about the colours some times, but guessed that it was my intuition that was wrong. Your example does look pretty odd though, so I reckon something is indeed wrong. I'll investigate.

jefferis commented 8 years ago

The problem is that target and query are swapped here. The colours that you are getting are for the segments in the query neuron but you are plotting them for the target neuron. It is confusing.

If you swap n1 and n2 here:

f(PlotVectors) {
    # We need to duplicate each colour as we are drawing line segments, not points
    segcols <- c(sapply(segcols, function(x) c(x,x)))
    plot3d(n2, col=col, PlotVectors=TRUE, PlotPoints=FALSE, ...)
    plot3d(n1, col=segcols, PlotVectors=TRUE, PlotPoints=FALSE, ...)
  } else {
    plot3d(n2, col=col, PlotVectors=FALSE, PlotPoints=TRUE, ...)
    plot3d(n1, col=segcols, PlotVectors=FALSE, PlotPoints=TRUE, ...)
  }

it should work.

jdmanton commented 8 years ago

Ah, that would explain why it looked better for more similar neurons... Sorry. Fixed in 5fdc16f465260ceac8b12cf7809c50a8be58222d.

jefferis commented 8 years ago

Cool. But do the docs need updating to clarify who will be coloured etc? And is it the comparison that makes most sense? I haven't thought it through completely myself.

jdmanton commented 8 years ago

Is it worth renaming n1 and n2 to target and query?

jefferis commented 8 years ago

Yes, I think that would be good.

jefferis commented 8 years ago

note also that the order of target and query args is different for nblast and WeightedNNBasedLinesetMatching.dotprops ...

jdmanton commented 8 years ago

note also that the order of target and query args is different for nblast and WeightedNNBasedLinesetMatching.dotprops ...

Yeah, I just noticed that... I've updated the docs for show_similarity in 8c03b201968726a506d64fa759a6239a6d0e5c54 so that it's consistent with the function's behaviour, but wonder if it would be worth swapping the target and query args to match up with nblast.

jdmanton commented 8 years ago

I've swapped the arguments in 80252653eca87827d4df7058e5ed0299d2fcf5ae. Now we compare the query to the target (and not the target to the query), as with nblast, and colour the query, which makes most sense to me.

jefferis commented 8 years ago

I think swapping the args is best so +1 for that.

Deciding which neuron should be coloured is more complicated. I think many people might expect the target to be coloured (because it is the hit, the thing that is being returned by the search). But in fact the per segment scores are really only defined for the query neuron. We could maybe give the option to colour either neuron, but that would involve mapping the scores for query segments onto the corresponding target segments.

So let's go with what you have now.

jdmanton commented 8 years ago

Should I submit this updated version to CRAN once I've fixed #31 too?

jefferis commented 8 years ago

Yes. But maybe wait a couple of days in case anything else surfaces.

jefferis commented 8 years ago

happy to close this issue now.