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

function for rerooting neuron #463

Merged
merged 14 commits into from Jun 5, 2021
Merged

function for rerooting neuron #463

merged 14 commits into from Jun 5, 2021

Conversation

dokato
Copy link
Contributor

@dokato dokato commented May 28, 2021

getting foot in the door.

TODO:

  • "And it should give the option to provide either indices or ids. See nat::distal_to for ideas."
  • Should have a generic of the form reroot(x,...)
  • and methods for neuron and neuronlist with arguments idx and pointno for both
  • neuronlist method needs to check argument length carefully
  • neuron method needs to check that the ids/indices reference a valid node
  • would also suggest that the neuron method returns the original object
  • finally we will need a specialised catmaidneuron method that ensures that additional metadata (synaptic connectors etc) is copied over when the neuron is remade
  • also add an entry in _pkgdown.yml
  • update news.md

@dokato
Copy link
Contributor Author

dokato commented May 28, 2021

and pointno for both

Here I decided to be more flexible and rather than point belonging to neuron I provide an option to reroot to a node that is closest to point

@dokato
Copy link
Contributor Author

dokato commented Jun 1, 2021

@jefferis I'd appreciate advice on how to go around this point:

finally we will need a specialised catmaidneuron method that ensures that additional metadata

It was tempting to just use a function from catmaid:

#' @export
reroot.catmaidneuron<-function(object, ...) {
  n = NextMethod()
  copy_tags_connectors(n, object)
}

But this should go to catmaid then? Here it'd introduce a dependency loop.

@jefferis
Copy link
Collaborator

jefferis commented Jun 1, 2021

Thanks @dokato! Yes, that was exactly what I was expecting the method should look like;reroot.catmaidneuron should be in catmaid. In future we should move some of the support for neurons with connectivity from catmaid into nat. But that will be a medium-sized change, so we should probably plan that separately.

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.

I would not run pkgdown as part of the PR. Could you remove those changes under docs. I've now started running pkgdown on github for new packages, but for now, prefer to run it periodically on master after changes have gone in.

R/neuron.R Outdated Show resolved Hide resolved
R/neuron.R Outdated Show resolved Hide resolved
R/neuron.R Outdated Show resolved Hide resolved
R/neuron.R Outdated Show resolved Hide resolved
R/neuron.R Outdated Show resolved Hide resolved
R/neuron.R Outdated Show resolved Hide resolved
R/neuron.R Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@dokato
Copy link
Contributor Author

dokato commented Jun 3, 2021

Thanks @jefferis for useful comments, I added support for matrices, node ids and removed pkgdown build. PTAL now.

@dokato
Copy link
Contributor Author

dokato commented Jun 3, 2021

BTW while on that I gave it a go and added reroot.catmaidneuron to https://github.com/natverse/rcatmaid

But for some reason I'm getting such error:

> conn=catmaid_login(server="https://l1em.catmaid.virtualflybrain.org")
> orns=read.neurons.catmaid("name:ORN right", .progress='text')
> class(orns[[1]])
[1] "catmaidneuron" "neuron"
> reroot.catmaidneuron(orns[[1]])
Error in NextMethod() : generic function not specified

I'm using nat version with the reroot changes ofkz. Maybe I miss something about generics, but I believe that I found some relevant example where the "next" method came from a different package - similar to our case. Any ideas what's wrong here?

@jefferis
Copy link
Collaborator

jefferis commented Jun 3, 2021 via email

@jefferis
Copy link
Collaborator

jefferis commented Jun 3, 2021 via email

@dokato
Copy link
Contributor Author

dokato commented Jun 3, 2021

ah damn, of course, that was it :P I'll send PR soon.

@jefferis
Copy link
Collaborator

jefferis commented Jun 3, 2021

Actually @dokato the generic should have signature (x, ...) and the method should have ... in addition to the named arguments. Generics should pretty much always have ... so that you can handle any subsequent methods that need other arguments.

NEWS.md Outdated Show resolved Hide resolved
R/interactive.R Outdated Show resolved Hide resolved
R/neuron.R Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Jun 3, 2021

Coverage Status

Coverage decreased (-0.06%) to 81.383% when pulling ca00daa on dokato:reroot into 760f19e on natverse:master.

R/neuron.R Outdated Show resolved Hide resolved
@jefferis
Copy link
Collaborator

jefferis commented Jun 3, 2021

@dokato I added some details to the docs. I'll merge when the checks complete.

@dokato dokato changed the title [WIP] function for rerooting neuron function for rerooting neuron Jun 4, 2021
@jefferis jefferis merged commit 88507a8 into natverse:master Jun 5, 2021
@dokato dokato deleted the reroot branch April 21, 2022 14:36
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.

None yet

3 participants