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

Skeletor #35

Merged
merged 74 commits into from Oct 9, 2020
Merged

Skeletor #35

merged 74 commits into from Oct 9, 2020

Conversation

alexanderbates
Copy link
Contributor

R wrapper for a skeleton pipeline in python to skeletonise neuron meshes, e.g. from FlyWire. Also, documentation and article describing how to install related python modules (updated version of Istvan Taiusz's flyconnectome slack post)

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.

Hi @alexanderbates, thanks for the PR. I have made a few comments implying changes. Could you respond to them or ping me if you have queries? Best, G.

savedir <- tempdir()
ff=file.path(savedir, paste0(id, '.obj'))
reticulate::py_run_string(sprintf("m.export('%s')",ff), ...)
res=sapply(ff, readobj::read.obj, convert.rgl = TRUE, simplify = FALSE)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since nat 1.10.2 (which is required by fafbseg) read.neurons will read meshes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

replaced

@@ -0,0 +1,10 @@
base
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexanderbates I'm not really sure about the wisdom of including rmarkdown caches here. Was this intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah no, it was not intentional. Somehow this was generated when I ran build_site().

You can gain access to the `Graphene/Chunkedgraph` server like below:

1. Visit https://globalv1.flywire-daf.com/auth/api/v1/refresh_token,
2. Copy the token present there, let's say it was "xxyyzz"
3. create a file named `chunkedgraph-secret.json` that has the following contents
3. create a file named `chunkedgraph-secret.json` at location `~.cloudvolume/secrets/chunkedgraph-secret.json`. For example, in Terminal:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo ~.cloudvolume/secrets/chunkedgraph-secret.json -> ~/.cloudvolume/secrets/chunkedgraph-secret.json

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorted

4. Save the file in the location `~.cloudvolume/secrets/chunkedgraph-secret.json`
5. Now you have access to the chunked graph server

You can also use the fafbseg function `set_chunkedgraph_token` to add your token to this file.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexanderbates I am about to change this function's name to flywire_set_token. Furthermore, I think there is really no point in this lengthy description as flywire_set_token/set_chunkedgraph_token does everything you need including getting the new token from the flywire website.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah okay! I just updated these old instructions a bit because i found it helpful

docs/pkgdown.yml Outdated
@@ -1,10 +1,10 @@
pandoc: 2.7.3
pkgdown: 1.5.1
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexanderbates can you please remove all changes to the docs/ folder. I would rather regenerate this myself after merging to master. Otherwise there are so many changes I find it hard to see the wood from the trees. There is however one thing that is missing, you could please edit _pkgdown.yml (note leading underscore) to add your new exported function(s). Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there an easy way to revert all changes within a specified folder?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jefferis and others added 4 commits September 23, 2020 16:28
* we shouldn't need to do this if we are skeletonising .obj files on disk
* 'skeletor' of github.com:natverse/fafbseg:
  Replace WHO with WH0
* for skeletonisation and radii calculation
jefferis and others added 3 commits October 1, 2020 21:41
* 'skeletor' of github.com:natverse/fafbseg:
  Update skeletor to work with more methods arguments

conflicts due to double edits including in errors where there were logic
changes on both sides that needed integrating
* 'skeletor' of github.com:natverse/fafbseg:
  Add theta argument for mesh cleaning w.r.t skeleton
@@ -21,7 +22,12 @@
#' You will need to have the \code{ncollpyde}
#' python3 module installed. You can get this with \code{pip3 install ncollpyde}. If you get issues
#' related to this module, best to set this to \code{FALSE}.
#' @param radius Logical. Whether or not to return radius information for each skeleton node.
#' @param theta numeric. Used if \code(clean==TRUE). For each twig we generate the dotproduct between the tangent
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexanderbates, this should be \code{clean=TRUE}. Can you fix?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you don't feel like running R CMD check before pushing (which you should), you should at least run devtools::check_man() to make sure that the docs aren't broken!

@jefferis
Copy link
Collaborator

jefferis commented Oct 5, 2020

@alexanderbates for reference a conditional test along these lines

test_that("skeletor works", {
  skip_if_not_installed('reticulate')
  cv_available=try(check_cloudvolume_reticulate, silent = TRUE)
  skip_if_not(cv_available, 'skipping tests requiring cloudvolume')
  res=skeletor(obj="testdata/720575940637333875.obj")
  # some more interesting expectation
  expect_is(res, 'neuron')
})

savedir <- if(!is.null(save.obj)){
save.obj
}else{
tempdir()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tempdir() is the root temporary folder of the R session. You should not delete it (as you found out the other day when you were deleting bridging registrations). It is better to make your own temporary folder

dir.create(td<-tempfile())
td

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexanderbates: you should also add an on.exit hook to delete that folder on exit

on.exit(unlink(td, recursive=TRUE))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, done

@@ -364,8 +364,7 @@ py_skeletor <- function(id,
if(reroot){
neuron = reroot_hairball(neuron, k.soma.search = k.soma.search, radius.soma.search = radius.soma.search, brain = brain)
}
if(mesh3d|save.obj){
if(is.null(mesh)){
if(mesh3d|!is.null(save.obj)){
Copy link
Collaborator

@jefferis jefferis Oct 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexanderbates: as a general rule it is better to use || or && for this kind of logic in if statements as those operators short circuit i.e. they stop evaluating when the value of the expression is certain. So if you do

if(shortcheck() | longcheck())

both are evaluated even if shortcheck() returns TRUE whereas

if(shortcheck() || longcheck())

would stop evaluating if shortcheck() returned TRUE

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want convincing, here is some code

fastcheck <- function(x=T) {
  message("fast")
  x
}

slowcheck <- function(x=T) {
  message("slow")
  x
}

message("|| operator")
if(fastcheck() || slowcheck()) {
  message("if!")
} else {
  message("else!")
}


message("| operator")
if(fastcheck() | slowcheck()) {
  message("if!")
} else {
  message("else!")
}
|| operator
fast
if!
| operator
fast
slow
if!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, I'll also apply this more generally.

@jefferis jefferis merged commit 416593a into master Oct 9, 2020
@alexanderbates alexanderbates deleted the skeletor branch October 10, 2020 14:40
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

2 participants