search

openjournals / joss-reviews

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

[REVIEW]: `pygwb`: a Python-based library for gravitational-wave background searches #5454

Closed editorialbot closed 6 months ago

editorialbot commented 1 year ago

Submitting author: !--author-handle-->@a-renzini<!--end-author-handle-- (Arianna Renzini) Repository: https://github.com/a-renzini/pygwb/ Branch with paper.md (empty if default branch): joss_submission Version: v1.4.0 Editor: !--editor-->@plaplant<!--end-editor-- Reviewers: @Sbozzolo, @cmbiwer Archive: 10.5281/zenodo.10532825

Status

status

Status badge code:

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

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

@sbozzolo, your review will be checklist based. Each of you will have a separate checklist that you should update when carrying out your review. First of all you need to run this command in a separate comment to create the checklist:

@editorialbot generate my checklist

The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @plaplant 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 ✨

Checklists

📝 Checklist for @Sbozzolo

📝 Checklist for @cmbiwer

editorialbot commented 1 year ago

Hello humans, I'm @editorialbot, a robot that can help you with some common editorial tasks.

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

@editorialbot commands

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

@editorialbot generate pdf
editorialbot commented 1 year ago

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

Sbozzolo commented 1 year ago

@editorialbot generate my checklist

editorialbot commented 1 year ago

@Sbozzolo I can't do that because you are not a reviewer

Sbozzolo commented 1 year ago

@Sbozzolo I can't do that because you are not a reviewer

@plaplant

Sbozzolo commented 1 year ago

Hi @a-renzini, I will be reviewing your package.

I wanted to clear some expectations regarding the timeline: I will soon leave for some travel, so I won't be able to work on the review for the next 3 weeks or so. I am sorry about that since I see that you've already waited for a month.

Even if the second reviewer hasn't been found yet, I would invite you to look at the review checklist and make sure that your project checks all the boxes.

From my cursory first look, I can already see some areas for improvement. In particular, the documentation seems particularly bare bones. Critically, the available documentation does not tell me what the code does nor how to use it. Here are some of the issues I've seen:

My personal rule of thumb to identify good documentation is to see if I could use the code just looking at the documentation. Currently, this is not possible. I will open an issue in your repo regarding this.

I am looking forward to diving into your package!

plaplant commented 1 year ago

@Sbozzolo thanks for your initial review! Apologies for the editorialbot command not working, please try it again when you get a chance.

Sbozzolo commented 1 year ago

Review checklist for @Sbozzolo

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

plaplant commented 1 year ago

@editorialbot add @cmbiwer as reviewer

editorialbot commented 1 year ago

@cmbiwer added to the reviewers list!

cmbiwer commented 1 year ago

I'll start reviewing and filling out the checklist on Friday.

cmbiwer commented 1 year ago

Review checklist for @cmbiwer

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

cmbiwer commented 1 year ago

I added some comments to the issue @Sbozzolo started here: https://github.com/a-renzini/pygwb/issues/1

I reviewed it without looking at @Sbozzolo review. And ended up with many of the same issues. I worked around some of the issues with the installation to try the test suite. I added a couple more things to that already existing issue on pygwb.

plaplant commented 1 year ago

@Sbozzolo @cmbiwer thank you so much for you initial reviews! I appreciate you making an issue on the upstream repo to keep track of specific suggestions You can also feel free to open separate issues for each individual suggestion, rather than having a single large issue.

@a-renzini please begin addressing the issues raised by the reviewers. Once you feel you have addressed them adequately, you can reply to this thread. Let me know if you have any questions!

arfon commented 1 year ago

@plaplant @dfm – just logging here that the AAS-associated paper is 10.3847/1538-4357/acd775

Sbozzolo commented 1 year ago

@a-renzini

The existence of two nearly identical public repositories (https://github.com/a-renzini/pygwb/, https://git.ligo.org/pygwb/pygwb) is a little bit confusing. Which one is the "real" one?

If you want to continue the development on GitLab but also want to be on GitHub, maybe you can consider mirroring the repository instead of creating a separate one?

a-renzini commented 1 year ago

@a-renzini

The existence of two nearly identical public repositories (https://github.com/a-renzini/pygwb/, https://git.ligo.org/pygwb/pygwb) is a little bit confusing. Which one is the "real" one?

If you want to continue the development on GitLab but also want to be on GitHub, maybe you can consider mirroring the repository instead of creating a separate one?

Hello @Sbozzolo - that is exactly what I've done; the GitHub is a mirror of the GitLab! We are currently developing on GitLab.

Sbozzolo commented 1 year ago

@a-renzini The existence of two nearly identical public repositories (https://github.com/a-renzini/pygwb/, https://git.ligo.org/pygwb/pygwb) is a little bit confusing. Which one is the "real" one? If you want to continue the development on GitLab but also want to be on GitHub, maybe you can consider mirroring the repository instead of creating a separate one?

Hello @Sbozzolo - that is exactly what I've done; the GitHub is a mirror of the GitLab! We are currently developing on GitLab.

It doesn't look like that the mirroring is working properly: the latest commit on GitLab is 36e6550eb5622d4456861b46c10c047163d394a5, but this commit does not exist on GitHub.

Also, if the repo on GitHub is just a mirror, I'd recommend pointing users to the GitLab repo in the readme (and maybe renaming the repo to pygwb-mirror, see, e.g., https://github.com/emacs-mirror/emacs). Your JOSS submission says that the link to the repo is the GitHub one, so maybe we should amend that too.

a-renzini commented 1 year ago

@a-renzini The existence of two nearly identical public repositories (https://github.com/a-renzini/pygwb/, https://git.ligo.org/pygwb/pygwb) is a little bit confusing. Which one is the "real" one? If you want to continue the development on GitLab but also want to be on GitHub, maybe you can consider mirroring the repository instead of creating a separate one?

Hello @Sbozzolo - that is exactly what I've done; the GitHub is a mirror of the GitLab! We are currently developing on GitLab.

It doesn't look like that the mirroring is working properly: the latest commit on GitLab is 36e6550eb5622d4456861b46c10c047163d394a5, but this commit does not exist on GitHub.

Also, if the repo on GitHub is just a mirror, I'd recommend pointing users to the GitLab repo in the readme (and maybe renaming the repo to pygwb-mirror, see, e.g., https://github.com/emacs-mirror/emacs). Your JOSS submission says that the link to the repo is the GitHub one, so maybe we should amend that too.

That is odd! I have just checked, and indeed the mirror failed a month ago. I will get that fixed asap.

I'll look into pointing/renaming as suggested. This does bring up a more top-level question though: during initial submission (which was a while ago now) we fixed the version for review to v1.0.0, which is the one associated to the ApJ paper. Shall we review this version for the JOSS paper, or shall we review the most up-to-date version? This will make a difference to the documentation. We don't have strong feelings either way; I don't know what the standard for JOSS papers is (whether the paper is linked to a specific version of the code, or whether it is meant to be somewhat general).

a-renzini commented 1 year ago

@a-renzini The existence of two nearly identical public repositories (https://github.com/a-renzini/pygwb/, https://git.ligo.org/pygwb/pygwb) is a little bit confusing. Which one is the "real" one? If you want to continue the development on GitLab but also want to be on GitHub, maybe you can consider mirroring the repository instead of creating a separate one?

Hello @Sbozzolo - that is exactly what I've done; the GitHub is a mirror of the GitLab! We are currently developing on GitLab.

It doesn't look like that the mirroring is working properly: the latest commit on GitLab is 36e6550eb5622d4456861b46c10c047163d394a5, but this commit does not exist on GitHub. Also, if the repo on GitHub is just a mirror, I'd recommend pointing users to the GitLab repo in the readme (and maybe renaming the repo to pygwb-mirror, see, e.g., https://github.com/emacs-mirror/emacs). Your JOSS submission says that the link to the repo is the GitHub one, so maybe we should amend that too.

That is odd! I have just checked, and indeed the mirror failed a month ago. I will get that fixed asap.

I'll look into pointing/renaming as suggested. This does bring up a more top-level question though: during initial submission (which was a while ago now) we fixed the version for review to v1.0.0, which is the one associated to the ApJ paper. Shall we review this version for the JOSS paper, or shall we review the most up-to-date version? This will make a difference to the documentation. We don't have strong feelings either way; I don't know what the standard for JOSS papers is (whether the paper is linked to a specific version of the code, or whether it is meant to be somewhat general).

I have now fixed the mirroring. sorry about that.

a-renzini commented 1 year ago

@Sbozzolo I realise this wasn't obvious: the new documentation is produced as a pages job in github, and lives here:

https://a-renzini.github.io/pygwb/

once we are happy with it, I will merge it into gitlab and it will be produced as a gitlab pages job.

Sbozzolo commented 1 year ago

I'll look into pointing/renaming as suggested. This does bring up a more top-level question though: during initial submission (which was a while ago now) we fixed the version for review to v1.0.0, which is the one associated to the ApJ paper. Shall we review this version for the JOSS paper, or shall we review the most up-to-date version? This will make a difference to the documentation. We don't have strong feelings either way; I don't know what the standard for JOSS papers is (whether the paper is linked to a specific version of the code, or whether it is meant to be somewhat general).

This is probably a question for @plaplant, but I'd personally say that it's a better use of your time to upgrade the most recent version as opposed to an outdated one.

@Sbozzolo I realise this wasn't obvious: the new documentation is produced as a pages job in github, and lives here:

https://a-renzini.github.io/pygwb/

once we are happy with it, I will merge it into gitlab and it will be produced as a gitlab pages job.

Thank you!

I'd also recommend changing the repository in your JOSS paper to your GitLab one.

plaplant commented 1 year ago

@Sbozzolo @a-renzini thanks for both of your contributions to the review process! I think it's best to focus on the most recent version of the code, given that the initial submission was several months ago and development has progressed since then. Most software repositories are understandably still under development (even post-JOSS publication!). We will make and tag a new release when the review process is completed, but it need not be the version that was initially submitted.

As for the question about GitLab vs. a GitHub mirror: per JOSS's submission guidelines, the software must be hosted at a place where users can open issues using accounts without manual approval. It seems that it not possible on the GitLab version of the code to do that (though please correct me if that's not the case!), so the JOSS review and paper should be for the GitHub mirrored version. I agree that a name change clearly indicating it as a mirror might be helpful, but I don't think it's a requirement for publication if that is not desirable for some particular reason.

a-renzini commented 1 year ago

@plaplant - ok thanks! that all makes sense to me, and re. the mirror, yes, that is why we submitted from GitHub. We will discuss internally whether a name change would be useful (we may end up migrating to GitHub, but the timescale of the migration is I think beyond the scope of this review...)

@Sbozzolo and @cmbiwer -- apologies for the delay on the checklist, we have been populating the API and tutorials (see e.g. the new simulator page https://a-renzini.github.io/pygwb/api/pygwb.simulator.html by Max and Kevin, whom I can't seem to be able to tag at the moment) and I will take on the task of re-doing the install guide and contribution guide next week. I have been traveling last week/this week and I will now be offline until next week, fyi.

plaplant commented 1 year ago

@a-renzini just checking in to see how the review is progressing. If you have addressed the reviewer changes and feel this is ready for another look, please reply to this thread letting us know. Let me know if you have any questions!

a-renzini commented 1 year ago

@plaplant and rev.s @Sbozzolo and @cmbiwer - apologies for the delay, it's the summer and we all seem to go offline/travel for conferences at different times. We have carried out a large amount of updates that are ready for review, but we are still missing some items. In the interest of time, I will provide a list here by end of day today of all the pages/items ready for a second look! Thanks all

plaplant commented 1 year ago

@a-renzini thanks for the update! No worries, the summer is a good time for everyone to recharge 🙂 Let us know when you feel things are ready for another look!

plaplant commented 1 year ago

@a-renzini hope the summer is going well! Just another check-in to see where things stand on the submission. Let me know if you have any questions!

plaplant commented 1 year ago

@a-renzini just checking in to see if there's been any movement on addressing the reviewers' comments. Let me know if you have any questions!

a-renzini commented 1 year ago

@plaplant We are currently addressing the fresh batch of comments from @Sbozzolo ! Apologies, I have been following the git issue on the repo itself and lost track of this one. We will be able to close the open MRs with edits addressing these recent comments very soon.

plaplant commented 1 year ago

@a-renzini thanks for the update! No worries, just wanted to make sure this was still moving forward 🙂

maxlalleman commented 10 months ago

Dear reviewers,

Apologies again for the delay. We just pushed through modifications to the code which should resolve most/all issues posted by @Sbozzolo and @cmbiwer here: https://github.com/a-renzini/pygwb/issues.. If you have any more questions and/or additional comments, please let us know.

Sbozzolo commented 10 months ago

Hi @maxlalleman, thank you very much for your efforts!

Would you be able to comment on the issues with the changes that you made to address them? That would make reviewing the changes easier.

Thanks!

plaplant commented 10 months ago

@maxlalleman thanks very much for the update!

@Sbozzolo @cmbiwer when you get a chance, please go through the linked issues to make sure everything has been addressed to your satisfaction. Thanks again for your reviews!

Sbozzolo commented 9 months ago

@a-renzini

I have one last question about the paper, and everything else looks good to me. The paper does not mention any other piece of software with features similar to pyGWB (open source or not). If other software exists to solve similar problems to pyGWB, you should mention and reference them and explain what pyGWB does new/differently. It nothing else exists, I think you should clearly say that no software that solves this problem exists. (This would fall within the "statement of need" bullet point).

Thank you!

a-renzini commented 9 months ago

@Sbozzolo excellent point - there are precise comparisons between pygwb and previous code(s) in the ApJ publication but nothing here. I added a (minimal) sentence and reference about this in the "statement of need".

Sbozzolo commented 9 months ago

@editorialbot generate pdf

editorialbot commented 9 months ago

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

Sbozzolo commented 9 months ago

@a-renzini Thank you! :)

It's green light on my end.

a-renzini commented 9 months ago

@editorialbot generate pdf

a-renzini commented 9 months ago

@a-renzini Thank you! :)

It's green light on my end.

Great, thanks @Sbozzolo !

How do we proceed now, @plaplant ?

editorialbot commented 9 months ago

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

plaplant commented 9 months ago

@Sbozzolo Thanks so much for your review! We appreciate all of the work put in.

@cmbiwer Thanks again for your work thus far! If you feel the authors have addressed all of your issues, please edit your reviewer checklist above to reflect that fact. If not, please respond listing additional concerns you might have about the submission.

@a-renzini to move forward with the submission, both reviewers need to sign off. When that's done, we can begin final acceptance checks.

cmbiwer commented 9 months ago

I should have time Friday to take a look again.

cmbiwer commented 9 months ago

I review it again. I think the authors addressed nearly everything.

A lot of the tutorial section is geared toward a LVC user or someone with access to the same environment. Its probably has limited usefulness to a non-LVC user.

But for the parts I was able to try out the functionality I opened two issues:

cmbiwer commented 9 months ago

Well, the authors did address the prior two issues, but as I continue on these tutorials I run into the following errors now:

plaplant commented 9 months ago

@cmbiwer thanks very much for your follow-up reviews!

@a-renzini when you get a chance, please address the additional issues raised (linked above). Let me know if you have any questions!

cmbiwer commented 8 months ago

The authors have addressed all my issues. I've completed the checklist and they've done everything for publication in my opinion.

plaplant commented 7 months ago

@cmbiwer thanks for your review!

@a-renzini now that both reviewers have approved the submission, we can move forward with the final acceptance checks. If the software has changed in the course of the review (excluding changes to the paper/manuscript itself), please make a new tagged release of the repository. After that, please archive it (on Zenodo, figshare, or some other long-term hosting platform). Once these are done, please post the new version number and DOI of the archive in this thread. Let me know if you have any questions!

a-renzini commented 7 months ago

Thanks @plaplant , and all!

I've created a pypi release of v1.4.0 : https://pypi.org/project/pygwb/1.4.0/ And created an updated zenodo entry: https://zenodo.org/records/10532825 DOI