[REVIEW]: NeuroDSP: A package for neural digital signal processing

whedon commented 5 years ago

Submitting author: @voytek (Bradley Voytek) Repository: https://github.com/neurodsp-tools/neurodsp Version: 2.0.0 Editor: @arokem Reviewer: @TomDLT Archive: 10.5281/zenodo.2643784

Status

Status badge code:

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

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) 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

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

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

The reviewer guidelines are available here: https://joss.theoj.org/about#reviewer_guidelines. Any questions/concerns please let @arokem know.

✨ Please try and complete your review in the next two weeks ✨

Review checklist for @TomDLT

Conflict of interest

[x] As the reviewer I confirm that I have read the JOSS conflict of interest policy and that there are no conflicts of interest for me to review this work.

Code of Conduct

[x] I confirm that I read and will adhere to the JOSS code of conduct.

General checks

[x] Repository: Is the source code for this software available at the repository url?
[x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
[x] Version: 2.0.0
[x] Authorship: Has the submitting author (@voytek) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?

Functionality

[x] Installation: Does installation proceed as outlined in the documentation?
[x] Functionality: Have the functional claims of the software been confirmed?
[x] Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
[x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
[x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
[x] Automated tests: Are there automated tests or manual steps described so that the function of the software can be verified?
[x] Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

[x] Authors: Does the paper.md file include a list of authors with their affiliations?
[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] References: Do all archival references that should have a DOI list one (e.g., papers, datasets, software)?

whedon commented 5 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @TomDLT it looks like you're currently assigned as the reviewer for this paper :tada:.

: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:

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

watching

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

whedon commented 5 years ago

Attempting PDF compilation. Reticulating splines etc...

whedon commented 5 years ago

:point_right: Check article proof :page_facing_up: :point_left:

TomDLT commented 5 years ago

General review

This Python package implements a collection of more-or-less standard signal processing functions used to analyze univariate neurophysiological time-series.

This package is well packaged, documented, licensed, and tested. It comes with installation instructions, contributing guidelines, and detailed tutorials. The API is clear, with consistent parameter names and orderings, and well documented.

I successfully installed the package and ran the tutorials with no effort. After a shallow code review and some personal testing, the implemented functions seems correct. The code is clear and complies with standard Python conventions.

Package content

The package first provides standard signal processing functions, such as spectral analysis, filtering, and Morlet decomposition. For these functions, it relies heavily on scipy.signal, and extends it nicely in different ways, such as simpler interfaces, user-friendly parameters checks, edge artifacts considerations, or bootstrapping options.

It also implements less standard signal processing techniques, such as lagged-coherence (Fransen et al, 2015), sliding window matching (Gips et al, 2017), or burst detection (reference needed).

Finally, it proposes a number of functions to simulate neurophysiological signals, with different signal and noise models. Some comments might be added to emphasize that these are only models and could lead to oversimplistic signals, compared to empirical neurophysiological signals.

Minor remarks

The authors state in the paper that this package "can easily be integrated with frameworks such as MNE". As this package only considers univariate time-series, one would need a bit of work to integrate it properly with MNE. A small tutorial would maybe help the user start in the right direction.
Andrew J Washington seems to be a significant contributor. Have you considered adding him in the author list ?
Some guidelines could be added, for instance linked in the README.md, for third parties wishing to report issues or seek support.

Other than these minor remarks, I consider that this submission does meet the JOSS criteria. @arokem @voytek

voytek commented 5 years ago

Thanks, @TomDLT! We'll get back to you regarding your points, soon, but we appreciate you taking the time to do this.

arokem commented 5 years ago

Thanks for the quick review!

@voytek : one potential typo on my reread of the manuscript. Should "1/f life components" be "1/f-like components"? Or is that a technical term I'm just not familiar with?

voytek commented 5 years ago

@voytek : one potential typo on my reread of the manuscript. Should "1/f life components" be "1/f-like components"? Or is that a technical term I'm just not familiar with?

Too funny. Living my 1/f life. Good catch, @arokem. Fixed.

voytek commented 5 years ago

The API is clear, with consistent parameter names and orderings, and well documented.

Thanks, @TomDLT! The lab's been very diligent about our software development.

It also implements less standard signal processing techniques, such as lagged-coherence (Fransen et al, 2015), sliding window matching (Gips et al, 2017), or burst detection (reference needed).

We have now added a reference to the dual-threshold burst algorithm: Feingold et al., PNAS 2015: Bursts of beta oscillation differentiate post-performance activity in the striatum and motor cortex of monkeys performing movement tasks.

Some comments might be added to emphasize that these are only models and could lead to oversimplistic signals, compared to empirical neurophysiological signals.

Excellent point. I've added the following comment to the paper: "Note that these simulations are designed to mimic the statistics of electrophysiological data—and properties of transient, non-stationary, non-sinusoidal rhythms—but they should not be over-interpreted as biophysically realistic."

The authors state in the paper that this package "can easily be integrated with frameworks such as MNE". As this package only considers univariate time-series, one would need a bit of work to integrate it properly with MNE. A small tutorial would maybe help the user start in the right direction.

This is an excellent suggestion. I've opened an issue to ensure that this gets addressed soon.

Andrew J Washington seems to be a significant contributor. Have you considered adding him in the author list ?

Andrew was indeed helpful, but did not contribute significantly enough to the intellectual aspects of this project to warrant co-authorship, but we now acknowledge his help. We added the following to the Acknowledgements section: "We thank Andrew J. Washington for his early code contributions to this project." Thank you for catching this!

Some guidelines could be added, for instance linked in the README.md, for third parties wishing to report issues or seek support.

We have added explicit mention in the project README of how to ask questions and file bug reports (using Github issues), as well as a note on how to contribute, with associated updates to the CONTRIBUTING file, explaining the ideal workflow - see pull request #137 on the neurodsp repository.

arokem commented 5 years ago

OK. Looks like all the boxes are checked and looks like the one remaining concern (MNE tutorial) is currently in progress here: https://github.com/neurodsp-tools/neurodsp/pull/146

Once that's merged, please ping @TomDLT to take another look and confirm that the review is complete.

TomDonoghue commented 5 years ago

Hey @TomDLT & @arokem

I've finished up a version of an example using MNE, as was suggested in the review, and the relevant PR has now been merged: https://github.com/neurodsp-tools/neurodsp/pull/146 You can also see the hosted version of this example here: https://neurodsp-tools.github.io/neurodsp/auto_examples/plot_mne_example.html#sphx-glr-auto-examples-plot-mne-example-py

One more thing - we have been working more generally on some updates for the module (see: https://github.com/neurodsp-tools/neurodsp/pull/150). Related to this, and to make sure the paper will still accurately reflect the module after these changes go through, we have some proposed updates to the JOSS paper, which are here: https://github.com/neurodsp-tools/neurodsp/pull/151

Basically: the upcoming changes include some code re-organizations / refactors (no big changes to most implementations), and the updates in that PR make the paper match a little closer to what the module will look like soon. That PR for the paper edits is basically ready to be merged - I just wasn't quite sure the protocol for us initiating changes on the paper through the review process, and wanted to mention it here first before merging!

If the MNE example looks good to y'all, and if the paper edits we recommend look okay, then I think we're done everything we have noted on our end for the JOSS paper - so please let us know if there is anything else we should do. Thanks!

cc: @voytek

TomDLT commented 5 years ago

Nice MNE example, and moving swm and lagged coherence in the same module makes sense. Everything is ok on my side.

TomDonoghue commented 5 years ago

Awesome - thanks for doing the review @TomDLT!

@arokem is it okay if I merge paper updates? And is there anything else you need from us?

arokem commented 5 years ago

Yes. Please do merge the paper updates and the updates. I think that it would be better for the paper to represent a more up-to-date version of the software. Once you do that, I'll take another look at the rendered PDF to make sure that everything is in order. Then, I'll ask you to deposit an archive with the accepted version of the software and article, and then, we can move on to ask the EIC for a final look and approval. Does that all make sense?

arokem commented 5 years ago

Hey @TomDonoghue : have you merged the last updates in?

TomDonoghue commented 5 years ago

Hey @arokem - sorry for the delay, we were just fixing up some last things in the sim module that got refactored in the updates - but yes, I did just merge the updates in!

What is now in the paper folder on master is the updated version of the paper (with the updates for the code updates), and all those code updates have now also been merged to master.

This means we should be ready to check the new rendered version of the article! After that, depositing an archive of the article / code makes sense - though I'm not sure how to do that / where to put it, if you can perhaps direct us on some further instructions on that!

cc: @voytek & @rdgao

arokem commented 5 years ago

@whedon generate pdf

whedon commented 5 years ago

Attempting PDF compilation. Reticulating splines etc...

arokem commented 5 years ago

@whedon check references

whedon commented 5 years ago

Attempting to check references...

whedon commented 5 years ago

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

Can't find any papers to compile :-(

whedon commented 5 years ago


OK DOIs

- 10.1126/science.1099745 is OK
- 10.1038/nrn3241 is OK
- 10.1101/452987 is OK
- 10.1101/302000 is OK
- 10.1073/pnas.1517629112 is OK
- 10.1016/j.neuroimage.2015.06.003 is OK
- 10.1007/s11571-008-9064-y is OK
- 10.1016/j.neuroimage.2017.06.078 is OK
- 10.1016/j.jneumeth.2016.11.001 is OK
- 10.1016/j.neuroimage.2013.10.027 is OK
- 10.1101/299859 is OK
- 10.1016/j.tics.2014.04.003 is OK
- 10.1371/journal.pcbi.1000609 is OK
- 10.1523/JNEUROSCI.2332-14.2015 is OK

MISSING DOIs

- None

INVALID DOIs

- None

arokem commented 5 years ago

@whedon generate pdf

whedon commented 5 years ago

Attempting PDF compilation. Reticulating splines etc...

whedon commented 5 years ago

:point_right: Check article proof :page_facing_up: :point_left:

arokem commented 5 years ago

Is the first sentence missing a word?

Should that be two sentences? Something like: "Populations of neurons exhibit time-varying fluctuations in their aggregate electrical activity. These fluctuations are measured using ectrophysiological methods, such as magneto or electroencephalography (M/EEG), intracranial EEG (iEEG) or electrocorticography (ECoG), and local field potential (LFP) recordings [@buzsaki_origin_2012]."

voytek commented 5 years ago

Sure was! It used to be correct, but looks like it got messed up down the line. Fixed. Please rebuild. Sorry, and thanks for catching it!

arokem commented 5 years ago

@whedon generate pdf

whedon commented 5 years ago

Attempting PDF compilation. Reticulating splines etc...

whedon commented 5 years ago

:point_right: Check article proof :page_facing_up: :point_left:

arokem commented 5 years ago

@whedon set version as 1.1.2

arokem commented 5 years ago

@TomDonoghue : to create an archive, please upload the software as it is now to a service such as zenodo that will create a doi for the software. Once you have that, please post the DOI here, and I will set that as the archive for the software.

arokem commented 5 years ago

@whedon set 1.1.2 as version

whedon commented 5 years ago

OK. 1.1.2 is the version.

TomDonoghue commented 5 years ago

Hey @arokem - thanks for the info, couple quick questions:

1) In the article proof, a couple of the in text citations are a little weird: specifically it cites B. Voytek in the first paragraph, and Scott R Cole in the second bullet point of the modules, inconsistently using first names / initials. It's not clear from looking at the paper.bib file what's different - do you happen to know what's making it do that?

2) The current version of the software is 2.0.0, though this is not yet tagged on Github and is a pre-release on pip (just so it gets a little more road tested after the revamp for 2.0). Can we set that as the software version number? That's the version that I will put up on Zenodo, if that all sounds good to you.

arokem commented 5 years ago

@whedon set 2.0.0 as version

whedon commented 5 years ago

OK. 2.0.0 is the version.

arokem commented 5 years ago

Your last bib item uses a different format for first/last names than all the others.

Also, I think you have "Scott R Cole" here:

https://github.com/neurodsp-tools/neurodsp/blob/master/paper/paper.bib#L41

And "Scott Robert Cole" here:

https://github.com/neurodsp-tools/neurodsp/blob/master/paper/paper.bib#L31

Which is also causing some issues.

TomDonoghue commented 5 years ago

Okay thanks for the info / guidance!

I stepped through and updated the references, trying to make sure that they all used the same format, so hopefully that fixed things up. Can you rebuild the paper?
I enabled Zenodo for the NeuroDSP repository, and tagged a pre-release version of 2.0.0, and it got assigned a DOI through Zenodo: DOI: 10.5281/zenodo.2643784 Zenodo badge: https://zenodo.org/badge/doi/10.5281/zenodo.2643784.svg

arokem commented 5 years ago

@whedon set 10.5281/zenodo.2643784 as archive

whedon commented 5 years ago

OK. 10.5281/zenodo.2643784 is the archive.

arokem commented 5 years ago

@TomDonoghue : I'll rebuild, but FYI: you can also ask whedon to do that.

arokem commented 5 years ago

@whedon generate pdf

whedon commented 5 years ago

Attempting PDF compilation. Reticulating splines etc...

whedon commented 5 years ago

:point_right: Check article proof :page_facing_up: :point_left:

arokem commented 5 years ago

The references look good to me. Please confirm and I will go ahead with the acceptance process.

voytek commented 5 years ago

Looks great! Thanks @arokem.

TomDonoghue commented 5 years ago

@whedon generate pdf

whedon commented 5 years ago

Attempting PDF compilation. Reticulating splines etc...

whedon commented 5 years ago

:point_right: Check article proof :page_facing_up: :point_left:

TomDonoghue commented 5 years ago

@whedon generate pdf

openjournals / joss-reviews