search

openjournals / joss-reviews

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

[REVIEW]: EMD: Empirical Mode Decomposition and Hilbert-Huang Spectral Analyses in Python #2977

Closed whedon closed 3 years ago

whedon commented 3 years ago

Submitting author: @ajquinn (Andrew Quinn) Repository: https://gitlab.com/emd-dev/emd Version: 0.4.0 Editor: @dpsanders Reviewer: @JanCBrammer, @EtienneCmb, @neurofractal Archive: 10.5281/zenodo.4647949

: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/02ba7be4eea16a18976d264ac5443dbd"><img src="https://joss.theoj.org/papers/02ba7be4eea16a18976d264ac5443dbd/status.svg"></a>
Markdown: [![status](https://joss.theoj.org/papers/02ba7be4eea16a18976d264ac5443dbd/status.svg)](https://joss.theoj.org/papers/02ba7be4eea16a18976d264ac5443dbd)

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

@JanCBrammer & @EtienneCmb & @neurofractal, 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 @dpsanders 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 @JanCBrammer

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @EtienneCmb

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @neurofractal

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 3 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @JanCBrammer, @EtienneCmb, @neurofractal 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
whedon commented 3 years ago

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

/app/vendor/bundle/ruby/2.6.0/bundler/gems/whedon-92346a0773a4/lib/whedon/author.rb:72:in block in build_affiliation_string': Problem with affiliations for Anna Nobre, perhaps the affiliations index need quoting? (RuntimeError) from /app/vendor/bundle/ruby/2.6.0/bundler/gems/whedon-92346a0773a4/lib/whedon/author.rb:71:ineach' from /app/vendor/bundle/ruby/2.6.0/bundler/gems/whedon-92346a0773a4/lib/whedon/author.rb:71:in build_affiliation_string' from /app/vendor/bundle/ruby/2.6.0/bundler/gems/whedon-92346a0773a4/lib/whedon/author.rb:17:ininitialize' from /app/vendor/bundle/ruby/2.6.0/bundler/gems/whedon-92346a0773a4/lib/whedon.rb:205:in new' from /app/vendor/bundle/ruby/2.6.0/bundler/gems/whedon-92346a0773a4/lib/whedon.rb:205:inblock in parse_authors' from /app/vendor/bundle/ruby/2.6.0/bundler/gems/whedon-92346a0773a4/lib/whedon.rb:202:in each' from /app/vendor/bundle/ruby/2.6.0/bundler/gems/whedon-92346a0773a4/lib/whedon.rb:202:inparse_authors' from /app/vendor/bundle/ruby/2.6.0/bundler/gems/whedon-92346a0773a4/lib/whedon.rb:93:in initialize' from /app/vendor/bundle/ruby/2.6.0/bundler/gems/whedon-92346a0773a4/lib/whedon/processor.rb:38:innew' from /app/vendor/bundle/ruby/2.6.0/bundler/gems/whedon-92346a0773a4/lib/whedon/processor.rb:38:in set_paper' from /app/vendor/bundle/ruby/2.6.0/bundler/gems/whedon-92346a0773a4/bin/whedon:58:inprepare' from /app/vendor/bundle/ruby/2.6.0/gems/thor-0.20.3/lib/thor/command.rb:27:in run' from /app/vendor/bundle/ruby/2.6.0/gems/thor-0.20.3/lib/thor/invocation.rb:126:ininvoke_command' from /app/vendor/bundle/ruby/2.6.0/gems/thor-0.20.3/lib/thor.rb:387:in dispatch' from /app/vendor/bundle/ruby/2.6.0/gems/thor-0.20.3/lib/thor/base.rb:466:instart' from /app/vendor/bundle/ruby/2.6.0/bundler/gems/whedon-92346a0773a4/bin/whedon:131:in <top (required)>' from /app/vendor/bundle/ruby/2.6.0/bin/whedon:23:inload' from /app/vendor/bundle/ruby/2.6.0/bin/whedon:23:in `

'

whedon commented 3 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1098/rspa.1998.0193 is OK
- 10.1109/icassp.2005.1416051 is OK
- 10.1142/s1793536909000096 is OK
- 10.1142/s1793536909000047 is OK
- 10.1098/rsta.2015.0206 is OK

MISSING DOIs

- None

INVALID DOIs

- None
AJQuinn commented 3 years ago

Hello - thanks all for your time!

I have the latest version of the paper in a branch for the moment, I think this is causing the whedon error above...

AJQuinn commented 3 years ago

@whedon generate pdf from branch 33-joss-revisions

whedon commented 3 years ago 
Attempting PDF compilation from custom branch 33-joss-revisions. Reticulating splines etc...
whedon commented 3 years ago

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

EtienneCmb commented 3 years ago

Hi @AJQuinn,

Some minor comments about the draft :

Small question : is the holospectrum related to phase-amplitude coupling and comodulogram plot?

EtienneCmb commented 3 years ago

You could also add one sentence to the manuscript to highlight that some computations can be accelerated though multi-processing. This is a nice feature that doesn't seems to be present over all other EMD packages

JanCBrammer commented 3 years ago

@whedon re-invite @JanCBrammer as reviewer

whedon commented 3 years ago

I'm sorry @JanCBrammer, I'm afraid I can't do that. That's something only editors are allowed to do.

JanCBrammer commented 3 years ago

I'm sorry @JanCBrammer, I'm afraid I can't do that. That's something only editors are allowed to do.

@dpsanders, could you re-invite me (invitation has expired)?

danielskatz commented 3 years ago

@whedon re-invite @JanCBrammer as reviewer

whedon commented 3 years ago

OK, the reviewer has been re-invited.

@jancbrammer please accept the invite by clicking this link: https://github.com/openjournals/joss-reviews/invitations

AJQuinn commented 3 years ago

Hi all, I've made some paper changes based on @EtienneCmb suggestions above. Please let me know if these are ok and if there are any more!

We followed up the point about phase-amplitude coupling elsewhere in the end but just in case others are interested... the Holospectrum quantifies any amplitude modulations present in an oscillatory signal but does not formally relate these to the phase of a low frequency signal. The methods measure complementary things but you would still need a phase x frequency plot or comodulogram alongside the holospectrum for a complete PAC measure (there is an example of this at the end of the Holospectrum tutorial)

Cheers, Andrew

AJQuinn commented 3 years ago

@whedon generate pdf from branch 33-joss-revisions

whedon commented 3 years ago 
Attempting PDF compilation from custom branch 33-joss-revisions. Reticulating splines etc...
whedon commented 3 years ago

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

whedon commented 3 years ago

:wave: @EtienneCmb, please update us on how your review is going.

whedon commented 3 years ago

:wave: @JanCBrammer, please update us on how your review is going.

whedon commented 3 years ago

:wave: @neurofractal, please update us on how your review is going.

dpsanders commented 3 years ago

Sorry about the automatic reminders; please let us know here if you have comments. Detailed technical questions, comments etc. are best done as an issue on the software repository with a link to this issue. Thanks!

JanCBrammer commented 3 years ago

@AJQuinn, I suggested a few small manuscript edits over at #AJQuinn/emd-mirror/pull/1. I enjoyed reading the paper. Helpful figures! Will dive into the package and docs in the coming days.

JanCBrammer commented 3 years ago

Below is a list of observations from my review (in progress). I labeled those points that ideally should be addressed before publication with TODO. Everything else are merely suggestions or remarks. I'll let you know once I've concluded the review.

General

Paper

Documentation

Code

When browsing through the repository I come across some small issues:

JanCBrammer commented 3 years ago

I concluded my review and would be happy to see @AJQuinn's paper on the emd package published in JOSS. The paper is well written and the package is of high quality (excellent documentation, automated tests and package management, good coding practices). I explored emd using time series containing cardiac physiology (inter-beat-intervals) and found the core functionality (computing IMF and HHT) working to my satisfaction. Once @AJQuinn has addressed the TODO items in my earlier post, I'm happy to sign off on my review.

EtienneCmb commented 3 years ago

Same here, I tested emd on the examples such as on neural data and everything worked as expected. Overall, I also agree that the examples written in a tutorial fashion are really clear.

From my side, all lights are green for publishing in JOSS as soon as @AJQuinn has addressed my few comments about the examples.

neurofractal commented 3 years ago

Hi all.. Should be able to look at this over the next week or so. Excited to try some EMD on MEG data.

AJQuinn commented 3 years ago

Thank you all - very interesting and useful suggestions in the reviews. I'll make those changes during the week and make a note here when they're done.

Cheers

AJQuinn commented 3 years ago

Hi all - changes based on comments from @EtienneCmb are summarised here: https://gitlab.com/emd-dev/emd/-/issues/42#note_513753774 . I'm working on changes from @JanCBrammer next - will upate early next week.

Cheers

AJQuinn commented 3 years ago

Hi again - changes based on @JanCBrammer's comments are on this branch: https://gitlab.com/emd-dev/emd/-/merge_requests/53 - thanks for the suggestions! Please let me know if there is anything else I have missed or can do.

There is now also an updated manuscript based on all the changes so far here: https://gitlab.com/emd-dev/emd/-/merge_requests/38

Thanks all!

AJQuinn commented 3 years ago

@whedon generate pdf from branch 33-joss-revisions

whedon commented 3 years ago 
Attempting PDF compilation from custom branch 33-joss-revisions. Reticulating splines etc...
whedon commented 3 years ago

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

JanCBrammer commented 3 years ago

Thanks, @AJQuinn! I've been browsing through the changes at https://gitlab.com/emd-dev/emd/-/merge_requests/53 and I'm absolutely happy with the way you addressed my comments. I think you've put together an amazing, high quality piece of software and I can't wait to find an excuse to use it :grin:

AJQuinn commented 3 years ago

Thanks @JanCBrammer - that's great to hear. I'm glad you liked the package. I'll merge that branch in the next day or so!

EtienneCmb commented 3 years ago

Hi @AJQuinn, I looked at the changes you made, especially the tutorial about how to use EMD to retrieve the PAC. I have to say that it is a fantastic tutorial, very clear and the figures clearly explain your point. Like all of the other examples, the progression in the difficulty is very pleasant to read.

About the rest of your commits, I believe you addressed all of my points, even minor ones like the html representation.

Thank you for this nice piece of software, I learned a lot about EMD through the examples, I'll definitely use it in my data analysis to dig into my time-series.

AJQuinn commented 3 years ago

Thank you @EtienneCmb, very helpfull comments.

The changes from both reviews are now merged into the main package on gitlab.com and the updates to the website are live. Thanks for all your time!

Cheers

AJQuinn commented 3 years ago

Hi @neurofractal - thanks for the comments. I've included the changes in the branches attached to this issue: https://gitlab.com/emd-dev/emd/-/issues/47

I'll recompile the paper with changes here as well - let me know if there is anything I've missed or anything else I can do!

Cheers

AJQuinn commented 3 years ago

@whedon generate pdf from branch 33-joss-revisions

whedon commented 3 years ago 
Attempting PDF compilation from custom branch 33-joss-revisions. Reticulating splines etc...
whedon commented 3 years ago

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

dpsanders commented 3 years ago

👋 @EtienneCmb and @JanCBrammer: Are you happy with the current version and finished with your reviews? Thanks!

dpsanders commented 3 years ago

@neurofractal: Could you please fill in the checkboxes at the top of this review? Thanks!

neurofractal commented 3 years ago

@dpsanders all done. Finished with my review!

JanCBrammer commented 3 years ago

Are you happy with the current version and finished with your reviews? Thanks!

@dpsanders, yes and yes 👍 Except for a tiny edit in the current proof: I believe on line 55 the (?) after obtain can be removed?

EtienneCmb commented 3 years ago

@EtienneCmb and @JanCBrammer: Are you happy with the current version and finished with your reviews? Thanks!

yes I finished with my review and I'm happy with the current version :+1:

AJQuinn commented 3 years ago

@whedon generate pdf from branch 33-joss-revisions

whedon commented 3 years ago 
Attempting PDF compilation from custom branch 33-joss-revisions. Reticulating splines etc...
whedon commented 3 years ago

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

AJQuinn commented 3 years ago

Hi @dpsanders - is there anything else outstanding for this review?

Sorry to ask... just wondering about timings. I'm close to needing the next release and would like to get all the JOSS changes in a single increment if possible. I'll wait if there are only minor changes needed but if there is something bigger required I'll plan for two releases. Thank you for your time