search

openjournals / joss-reviews

Reviews for the Journal of Open Source Software
Creative Commons Zero v1.0 Universal
721 stars 38 forks source link

[REVIEW]: BrainGlobe Atlas API: a common interface for neuroanatomical atlases #2668

Closed whedon closed 4 years ago

whedon commented 4 years ago

Submitting author: @adamltyson (Adam Luke Tyson) Repository: https://github.com/brainglobe/bg-atlasapi/ Version: v1.0.0 Editor: @oliviaguest Reviewer: @typically, @vitay Archive: 10.5281/zenodo.4065389

:warning: JOSS reduced service mode :warning:

Due to the challenges of the COVID-19 pandemic, JOSS is currently operating in a "reduced service mode". You can read more about what that means in our blog post.

Status

status

Status badge code:

HTML: <a href="https://joss.theoj.org/papers/777458c96dffe8499c0d13886ecfe9bf"><img src="https://joss.theoj.org/papers/777458c96dffe8499c0d13886ecfe9bf/status.svg"></a>
Markdown: [![status](https://joss.theoj.org/papers/777458c96dffe8499c0d13886ecfe9bf/status.svg)](https://joss.theoj.org/papers/777458c96dffe8499c0d13886ecfe9bf)

Reviewers and authors:

Please avoid lengthy details of difficulties in the review thread. Instead, please create a new issue in the target repository and link to those issues (especially acceptance-blockers) by leaving comments in the review thread below. (For completists: if the target issue tracker is also on GitHub, linking the review thread in the issue or vice versa will create corresponding breadcrumb trails in the link target.)

Reviewer instructions & questions

@typically & @vitay, please carry out your review in this issue by updating the checklist below. If you cannot edit the checklist please:

  1. Make sure you're logged in to your GitHub account
  2. Be sure to accept the invite at this URL: https://github.com/openjournals/joss-reviews/invitations

The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @oliviaguest know.

✨ Please start on your review when you are able, and be sure to complete your review in the next six weeks, at the very latest ✨

Review checklist for @typically

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @vitay

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 4 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @typically, @vitay it looks like you're currently assigned to review this paper :tada:.

:warning: JOSS reduced service mode :warning:

Due to the challenges of the COVID-19 pandemic, JOSS is currently operating in a "reduced service mode". You can read more about what that means in our blog post.

:star: Important :star:

If you haven't already, you should seriously consider unsubscribing from GitHub notifications for this (https://github.com/openjournals/joss-reviews) repository. As a reviewer, you're probably currently watching this repository which means for GitHub's default behaviour you will receive notifications (emails) for all reviews 😿

To fix this do the following two things:

  1. Set yourself as 'Not watching' https://github.com/openjournals/joss-reviews:

watching

  1. You may also like to change your default settings for this watching repositories in your GitHub profile here: https://github.com/settings/notifications

notifications

For a list of things I can do to help you, just type:

@whedon commands

For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:

@whedon generate pdf
oliviaguest commented 4 years ago

@whedon generate pdf from branch joss

whedon commented 4 years ago 
Attempting PDF compilation from custom branch joss. Reticulating splines etc...
whedon commented 4 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

oliviaguest commented 4 years ago

Hey 👋 @typically, @vitay: this is where the review will take place. Please make sure to read the instructions above.

For any and all things worthy of discussion or comment, use this issue right here — so drop comments or questions for me, the author, etc., here. For any very code-specific things please feel free to start an issue on the repo of the code itself (if appropriate!) and link back to it from here. For an example of how this process plays out feel free to skim previous reviews, such as: #2285 and #2348. ☺️

vitay commented 4 years ago

@adamltyson just a small comment on the paper: the Winnubst et al. 2019 reference could use a doi.

oliviaguest commented 4 years ago

@whedon check references

oliviaguest commented 4 years ago

@whedon check references

oliviaguest commented 4 years ago

@whedon generate pdf from branch joss

whedon commented 4 years ago 
Attempting PDF compilation from custom branch joss. Reticulating splines etc...
whedon commented 4 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

oliviaguest commented 4 years ago

@openjournals/dev why is the check references command not working? Is it related to the fact it's on a custom branch?

danielskatz commented 4 years ago

@whedon check references from branch joss

whedon commented 4 years ago 
Attempting to check references... from custom branch joss
whedon commented 4 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.3389/fninf.2015.00011 is OK
- 10.1038/s41467-019-13057-w is OK
- 10.1002/cne.24080 is OK
- 10.1002/cne.24080 is OK
- 10.1073/pnas.0604911103 is OK
- 10.7554/eLife.53350 is OK
- 10.1016/j.cell.2020.04.007 is OK
- 10.1016/j.neuron.2019.04.034 is OK
- 10.1101/2020.02.23.961748 is OK
- 10.5281/zenodo.3991718 is OK

MISSING DOIs

- None

INVALID DOIs

- None
FedeClaudi commented 4 years ago

Hi @vitay I've added the DOI: https://github.com/brainglobe/bg-atlasapi/commit/dbe6a38ab3a8d692cfd16f74c88e086347fee1d2, thank you for pointing that out.

vitay commented 4 years ago

I guess I am already done with the review:

In short, I recommend acceptance.

oliviaguest commented 4 years ago

Hi @typically can you give us an ETA for your review, please? ☺️

typically commented 4 years ago

Hi @typically can you give us an ETA for your review, please? ☺️

Hi @oliviaguest, I should be able to get to this later next week, once students get sorted (it's orientation week here). It's on my radar!

oliviaguest commented 4 years ago

@whedon remind @typically in 16 days

whedon commented 4 years ago

Reminder set for @typically in 16 days

oliviaguest commented 4 years ago

@typically OK, I set-up a realistic reminder (for both of us to stay on track) given your plan then — thank you. 😊

typically commented 4 years ago

Okay, done reviewing. In summary, I love the tool, especially its simplicity and extensibility. I could definitely see how this could be integrated into any number of projects, including my own.

There are good basic examples of how to use the tool, but as minor suggestions:

  1. It might be useful to demonstrate with a practical example how to deal with coordinates (Cartesion, image indices). There is a reference to another package, but it isn't super clear to me how I would go from a downloaded atlas to some standard coordinate system.
  2. Hierarchical information appears to be provided (in the form of "structure_id_path"), and discussed in the paper. It would be useful to provide an example of how to use this information to obtain parcellations at different levels of hierarchy. It is not clear to me how this information can be represented, as the "annotation" image contains only a single integer per voxel.

I also recommend acceptance.

adamltyson commented 4 years ago

Hi @typically, thanks for your review!

For your first suggestion, I'm not 100% clear what you meant, do you mean something like: 1) How the voxel spacing in the images corresponds to the "real world"? i.e. showing that you can query the different resolution atlases in microns, and get the same results? And also show that the generated meshes for each brain region are in micron units (not voxels).

or

2) How to get your own data into the standard coordinate system? e.g. using brainreg to register sample image data to the atlas template image, and then analyse your data in the common coordinate space?

typically commented 4 years ago

Hi @adamltyson, I'm thinking of human templates, but more generally stereotaxic spaces. How would one map an image (specified in voxel indices) to Cartesian coordinates such as MNI-152, Talairach space, or rat stereotaxic coordinates, relative to bregma? You would need an origin and a basis vector, I assume. Just a suggestion though!

(So probably closer to 1.) :)

oliviaguest commented 4 years ago

@adamltyson I will leave you to think about any changes you might want to make before getting this officially accepted and published. So just drop a message here if you plan to do that, etc. 💯

adamltyson commented 4 years ago

Thanks for clarifying @typically. We don't have any method for converting between coordinate systems, but this would certainly be useful. Currently we only have one coordinate system for each species (e.g. each of the mouse atlases uses the same coordinates). If there's interest (and data available) for atlases with additional coordinate spaces, we could also provide methods to transform coordinate between coordinate spaces.

So, if it's alright with you, suggestion 1 will go onto our todo list, but it will depend on which atlases we add. We will document suggestion 2, as these methods exist in the software, but are not fully documented yet.

@oliviaguest, we'll add these small changes to the tutorials, then I think we'll be ready to publish.

typically commented 4 years ago

Hi @adamltyson, sounds good to me! Thanks for making this tool available. :)

typically commented 4 years ago

(sorry, closed inadvertently)

vigji commented 4 years ago

@typically now both the tutorial notebook and the Usage section of the docs here discuss more diffusely the hierarchy and the querying by index and/or coordinates in microns. As for the mapping from one atlas to the other, pure translations of the origin and changes in resolution and axes order can be done using https://github.com/brainglobe/bg-space, but we currently don't provide general support from affine/nonaffine transformations between spaces.

adamltyson commented 4 years ago

Hi @oliviaguest, as long as @vitay and @typically are happy, I think we're good to go now!

adamltyson commented 4 years ago

Hold that thought. We forgot to add an acknowledgments section.

typically commented 4 years ago

@vigji - Looks great! Re. spaces, was just curious but no need to add if it's not yet supported! Quite happy for it to be published.

vigji commented 4 years ago

@vigji - Looks great! Re. spaces, was just curious but no need to add if it's not yet supported! Quite happy for it to be published.

In any case, if you or other users are interested in trying the package out and would have this requirement for their applications we are more then happy to help out and implement it!

adamltyson commented 4 years ago

@whedon generate pdf from branch joss

whedon commented 4 years ago 
Attempting PDF compilation from custom branch joss. Reticulating splines etc...
whedon commented 4 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

adamltyson commented 4 years ago

We've made some small edits to the paper (affiliations, acknowledgments, reference formatting), and we're now good to publish. @oliviaguest what comes next?

oliviaguest commented 4 years ago

@adamltyson perfect — firstly, you need to deposit the code, so create an archive (on Zenodo, figshare, or other) and post the archive DOI here.

adamltyson commented 4 years ago

Thanks @oliviaguest, here's the DOI: 10.5281/zenodo.4065389

oliviaguest commented 4 years ago

@whedon set 10.5281/zenodo.4065389 as archive

whedon commented 4 years ago

OK. 10.5281/zenodo.4065389 is the archive.

oliviaguest commented 4 years ago

Great! @adamltyson I think the title of the repo on zenodo has to be the same as the title of the JOSS paper — change that when you get a moment. 😊

oliviaguest commented 4 years ago

@whedon check references

adamltyson commented 4 years ago

Great! @adamltyson I think the title of the repo on zenodo has to be the same as the title of the JOSS paper — change that when you get a moment.

Done

oliviaguest commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago

PDF failed to compile for issue #2668 with the following error:

Can't find any papers to compile :-(

oliviaguest commented 4 years ago

@whedon generate pdf from branch joss

whedon commented 4 years ago 
Attempting PDF compilation from custom branch joss. Reticulating splines etc...
whedon commented 4 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left: