[REVIEW]: Spectrum: Spectral Analysis in Python

whedon commented 7 years ago

Submitting author: @cokelaer (Thomas Cokelaer) Repository: https://github.com/cokelaer/spectrum Version: 0.7.1 Editor: @arfon Reviewer: @eteq Archive: 10.5281/zenodo.1037268

Status

Status badge code:

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

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 questions

@eteq, please carry out your review in this issue by updating the checklist below (please make sure you're logged in to GitHub). The reviewer guidelines are available here: http://joss.theoj.org/about#reviewer_guidelines. Any questions/concerns please let @arfon know.

Conflict of interest

[x] As the reviewer I confirm that there are no conflicts of interest for me to review this work (such as being a major contributor to the software).

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: Does the release version given match the GitHub release (0.6.2)?
[x] Authorship: Has the submitting author (@cokelaer) made major contributions to the software?

Functionality

[x] Installation: Does installation proceed as outlined in the documentation?
[x] Functionality: Have the functional claims of the software been confirmed?
[x] Performance: Have any performance claims of the software been confirmed?

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 7 years ago

Hello human, I'm @whedon. I'm here to help you with some common editorial tasks for JOSS. @eteq 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 as reviewer, you're probably currently watching this repository which means for GitHub's default behaviour you will receive notifications (emails) for all JOSS 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

arfon commented 7 years ago

@eteq - please carry out your review in this issue by updating the checklist above and giving feedback in this issue. The reviewer guidelines are available here: http://joss.theoj.org/about#reviewer_guidelines

Any questions/concerns please let me know.

cokelaer commented 7 years ago

FYI I have added the paper.md and paper.bin in the ./doc/joss directory of the github repository

arfon commented 7 years ago

Friendly reminder on this @eteq 😁

eteq commented 7 years ago

I've completed my review, @arfon and @cokelaer. Note that in the process I created a few issues/PRs in the repository itself (cokelaer/spectrum#31, cokelaer/spectrum#32, cokelaer/spectrum#33, cokelaer/spectrum#34, cokelaer/spectrum#35), but only the first two are really required (they are minor installation/install instructions quirks).

However, there are a number of more general items I found that need attention while working through the checklist. I've detailed those items below

[x] Both the paper and the documentation do not have a clear "statement of need". That is, they both explain what the package does, but not what it's for. I realize that power spectra are quite generally useful across the sciences and engineering, but it's in the JOSS review criteria, and I think it's quite useful to clarify what domain this has been designed for - i.e., the "target audience".
[x] Along similar lines: the examples are not "real world". They are all abstract mathematical exercises, even though power spectra are most useful when applied to real data, whether an engineering signal or a scientific time series. While not strictly mandatory, an example or two that either uses or simulates a "real world" case would improve the documentation a lot.
[x] There are no clear community guidelines. The contribution section simply says to look at the github cite, but says nothing about whether one should raise issues, create PRs, what sort of credit this leads to, etc. (This rolls into the next bullet point for probably obvious reasons) A github-style CONTRIBUTING.md (or rst) document would be an easy way to resolve this.
[x] There is only one author of the paper (@cokelaer), but the commit logs clearly show ~5 other contributors. If I understand right, these should also be coauthors of the JOSS paper (@arfon, is that correct?). And even if not, they should be referenced somewhere in the docs as contributors - currently there's a "by reference" that simply points to the github contributors interface, which is not necessarily a reliable measure (some contributors might not have github accounts, etc).
[x] While the various packages are a bit hazy on citation expectations, it seems like this work should cite numpy, matplotlib, and possibly scipy given that all are requirements and critical to the functioning of this package. Some guidelines for that are here

And one final item that doesn't necessarily require action, but might be worth considering now:

[ ] I found spectrum to be a rather confusing name. It has a lot of overloaded meanings in a lot of different scientific contexts. I realize this ship has probably sailed, but I thought I'd at least bring it up: have you (@cokelaer) considered using a more specific name like "power_spectrum"? I know plenty of people who wouldn't be able to use this solely because they have their own local package named spectrum and it would lead to a package namespace conflict...

I think the above puts this at "needs minor revision" (or at least arguments/explanations for why revision is not required).

arfon commented 7 years ago

Many thanks for your thorough review @eteq!

If I understand right, these should also be coauthors of the JOSS paper (@arfon, is that correct?)

We don't police authorship on JOSS submissions but yes, I think it would be appropriate to acknowledge the other contributors, perhaps by linking to the GitHub contribution graph? https://github.com/cokelaer/spectrum/graphs/contributors

eteq commented 7 years ago

Thanks for the clarification, @arfon - actually this one does link to the graph from the docs/README. But probably it should also be linked in the paper then?

In addition, I'd encourage @cokelaer to consider what spectrum's policy on authorship should be when addressing the contributor/community guidelines. I don't know if this has happened here, but in some cases the contributor graph doesn't properly catch everyone (e.g. commits with github-unrecognized email addresses). But as long as there's a clear understanding, all is fine.

We don't police authorship on JOSS submissions

Perhaps a discuss for elsewhere, or an already-had discussion (feel free to point me where if so, @arfon), but I wonder if there should be a policy on this?

danielskatz commented 7 years ago

@eteq - there has been a lot of research about authorship and contributorship in general, and some for software specifically.

One project worth noting is Project CRediT which arose out of trying to understand and then standardize contributor roles in biomedical papers. Of the 14 different possible roles that it found, some include writing text, but many do not. Similarly, in a software project, I think there are roles that lead to commits, and other roles that do not. And some committers, e.g. an administrator who adds a license file upon instruction from the project lead, may not really be contributors to the project.

Since the set of contributors is not the same as the set of committers, I don't believe that JOSS should make any general assumptions about who the contributors are based on who the committers are, but rather, the submitting author should tell us who the contributors are, and the editor and reviewer should point out any problems they see, such as you have brought up here. Maybe this is a new point we need to add to the reviewer checklist, as part of the authorship item. I've created a PR for this: https://github.com/openjournals/joss/pull/313

eteq commented 7 years ago

Thanks for the links, @danielskatz - I know some of that work but not that one.

I agree that it's not JOSS's role to set specific authorship rules, but it might be sensible to have some guidelines. This is particularly relevant because the review for software like this can be cross-disciplinary, where it's not clear what community's "rules" apply. For example, in my science domain I'd say it is inappropriate to not include code contributors. But I don't know for sure that the author of this package is in my domain. As you can see above, this lead me to phrase this as "consider what the right policy is", but it would be useful if JOSS had some guidelines about what I should do in this sort of situation so I'm not making this up as I go along :wink:.

cokelaer commented 7 years ago

@eteq @arfon thanks alot for the review. This will help improving the software indeed. I won't go through all the reviews right now but will come back to you very soon. Just a quick comment on the contributors. I am aware of the contributions and had put an issue to remind myself to add a list of contributors I let you know once I have an updated version.

danielskatz commented 7 years ago

@eteq - My opinion, for what it's worth, is that asking reviewers to check on this is probably the best we can do for now, to ensure that other reviewers are as conscientious as you have been, and if there's a question they have, they bring it up.

cokelaer commented 7 years ago

@eteq @arfon Here are some answers to the checklist.Thanks again for the suggestions/comments

no clear statement of need: I have edited the README.rst file adding

"The targetted audience is diverse. Although the use of power spectrum of a signal is fundamental in electrical engineering (e.g. radio communications, radar), it has a wide range of applications from cosmology (e.g., detection of gravitational waves in 2016), to music (pattern detection) or biology (mass spectroscopy)."

This is also reflected in the readthedocs documentation and paper.md for JOSS.

examples are not "real world" examples. Indeed, mostly used a complex (math) example and in some cases a sinus signal (one frequency). Although note that in the quick start example, this is a real-case example with the detection of a mono frequency signal. To improve this aspect of the documentation, I have added a gallery that will be completed little by little (http://pyspectrum.readthedocs.io/en/latest/auto_examples/index.html). For now, I have put 3 examples. One is a real case example (time-frequency detection of a dolphin "song") and the two others are still a bit theoretical (addition of two sinus signals).
No clear community guidelines: I have added a contributing.rst file
only one author on the paper: I have added an AUTHORS.rst with the link to the github account of the contributors. As for the JOSS paper, I have only added my name. This is a tricky question. I believe that most of the original work back from 2011 was done by myself and then small contributions were done by a few developers. I'm not sure how to handle this. Shall I add someone for fixing a typo in a comment for instance ? Where to you put the threshold ? Also as you said, what about personnal communication and bug report by email ? So, for now, I decided to go for a single author in the joss document while adding contributors in a dedicated file. I am open to discussion of course, but I think it is fair as it is. I could also contact the main contributors to ask their opinion.
"While various packages... citations...": I have added numpy/matplotlib/scipy in the paper.bib and added a short sentence at the end to explain what is used as dependencies.
"I found spectrum to be a rather confusing name": spectrum is quite generic indeed but used by a few people so I will stick to it.

cokelaer commented 7 years ago

Note also that since last two weeks, I have cleaned the code significantly, fixing a few bugs and adding tools such as spectrogram. These updates are available in the release 0.7.1 so we may want to update the version 0.6.2 to 0.7.1 in the JOSS review. thanks

arfon commented 7 years ago

@cokelaer are you ready for @eteq to take another look at your changes?

cokelaer commented 7 years ago

@arfon @eteq Yes, I had updated the code and documentation. please go ahead. thanks

arfon commented 7 years ago

@eteq - over to you.

arfon commented 7 years ago

Friendly reminder on this @eteq

eteq commented 7 years ago

@cokelaer - thanks for your work here. You've addressed all but one of my concerns sufficiently. On the one we're almost there, but I want to iterate once more. (But want to make it clear at the outset that I am not taking a hard-line stance and am open to discussion up to and including accepting everything as-is):

As for the JOSS paper, I have only added my name. This is a tricky question. I believe that most of the original work back from 2011 was done by myself and then small contributions were done by a few developers. I'm not sure how to handle this. Shall I add someone for fixing a typo in a comment for instance ? Where to you put the threshold ? Also as you said, what about personnal communication and bug report by email ? So, for now, I decided to go for a single author in the joss document while adding contributors in a dedicated file. I am open to discussion of course, but I think it is fair as it is. I could also contact the main contributors to ask their opinion.

My usual stance inside my field is that anyone who has contributed code or significant non-code effort should be a co-author of a package. Email communication/bug reports in general I wouldn't count, unless they involve substantive effort. I.e., someone emailed you a patch because they're not git-fluent, but if they had been, they would have issued a PR... Nut I would not count someone who emailed you once "hey maybe you should add feature X because I like it". And it seems to me that the "package authors" = "JOSS paper authors", given that if they deserve credit for the software, they should get some credit through a peer review service for the software (roughly speaking, that's JOSS).

All that said, I recognize all this is driven quite a bit by the social norms of my discipline: in astronomy we tend to have long author lists but recognize that the first (or at least a small subset) of the authors gets most of the credit for doing the work. In that scheme, having lots of extra authors isn't a problem and it's sort of rude not to include them. In other fields I know this is not the case. So if you think this is not the norm for your field, I'll accept that, but I want to be sure you've thought about it in those terms. And if you're not sure on that basis, I think the "ask the main contributors" is a good solution (although with a timeline - e.g. if they don't respond within a week or something, they forfeit the authorship option).

arfon commented 7 years ago

@cokelaer @eteq - Facing a similar issue, @adrn opened an issue in his repository to check with other contributors https://github.com/adrn/gala/issues/100

All that said, I recognize all this is driven quite a bit by the social norms of my discipline: in astronomy we tend to have long author lists but recognize that the first (or at least a small subset) of the authors gets most of the credit for doing the work. In that scheme, having lots of extra authors isn't a problem and it's sort of rude not to include them. In other fields I know this is not the case. So if you think this is not the norm for your field, I'll accept that, but I want to be sure you've thought about it in those terms. And if you're not sure on that basis, I think the "ask the main contributors" is a good solution (although with a timeline - e.g. if they don't respond within a week or something, they forfeit the authorship option).

To be clear, JOSS is not here to police authorship and I think @eteq has done a good job of articulating his position here. @cokelaer - if you're interested in doing something similar to @adrn in https://github.com/adrn/gala/issues/100 then we can wait for a week or so before accepting, otherwise, we can move forward with accepting this submission. Your choice 😸

cokelaer commented 7 years ago

@arfon @eteq I think that it is a valid point and agree with the idea of adding authors with significant contributions. I have open an issue accordingly https://github.com/cokelaer/spectrum/issues/41

eteq commented 7 years ago

Fantastic! Once your deadline from cokelaer/spectrum#41 has passed, you can consider me as signed off on this as accepted, @arfon and @cokelaer

arfon commented 7 years ago

Fantastic! Once your deadline from cokelaer/spectrum#41 has passed, you can consider me as signed off on this as accepted, @arfon and @cokelaer

@cokelaer - are we good to proceed?

cokelaer commented 7 years ago

I've asked two contributors to update the metadata related to authorship. Let us wait a couple of days. thanks

cokelaer commented 7 years ago

@arfon @eteq

I have added one additional author in the paper following public discussion at https://github.com/cokelaer/spectrum/issues/41. I believe we are ready to proceed.

arfon commented 7 years ago

@cokelaer - thanks for the update. Could you please merge this PR with some small fixes https://github.com/cokelaer/spectrum/pull/43 ? Also, please make sure you're citing the entries in your paper.bib file correctly (looks like they need a @ before the bibtex key) otherwise they won't be compiled into the final PDF. You can read how to do that here

cokelaer commented 7 years ago

@arfon should be fixed now.

arfon commented 6 years ago

Thanks @cokelaer. At this point could you make an archive of the reviewed software in Zenodo/figshare/other service and update this thread with the DOI of the archive? I can then move forward with accepting the submission.

cokelaer commented 6 years ago

@arfon, zenodo was already setup. I have just tagged a new release https://github.com/cokelaer/spectrum/releases that is now also on zenodo with the following DOI: https://doi.org/10.5281/zenodo.1037268

arfon commented 6 years ago

@whedon set 10.5281/zenodo.1037268 as archive

whedon commented 6 years ago

OK. 10.5281/zenodo.1037268 is the archive.

arfon commented 6 years ago

@eteq - many thanks for your review here ✨

@cokelaer - your paper is now accepted into JOSS and your DOI is https://doi.org/10.21105/joss.00348 ⚡️ 🚀 💥

cokelaer commented 6 years ago

brilliant thanks a lot for the review

On 26 October 2017 at 20:16, Arfon Smith notifications@github.com wrote:

@eteq https://github.com/eteq - many thanks for your review here ✨

@cokelaer https://github.com/cokelaer - your paper is now accepted into JOSS and your DOI is https://doi.org/10.21105/joss.00348 ⚡️ 🚀 💥

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/348#issuecomment-339753251, or mute the thread https://github.com/notifications/unsubscribe-auth/AAviRTTLSC7VWnHE56682j6u8M9GpmF_ks5swMyEgaJpZM4OrbsQ .

openjournals / joss-reviews