openjournals / joss-reviews

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

[REVIEW]: parafields: A generator for distributed, stationary Gaussian processes #5735

Closed editorialbot closed 11 months ago

editorialbot commented 1 year ago

Submitting author: !--author-handle-->@dokempf<!--end-author-handle-- (Dominic Kempf) Repository: https://github.com/parafields/parafields Branch with paper.md (empty if default branch): joss-paper Version: v1.0.1 Editor: !--editor-->@diehlpk<!--end-editor-- Reviewers: @shahmoradi, @gchure Archive: 10.5281/zenodo.10355636

Status

status

Status badge code:

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

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

@max-little & @shahmoradi, 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 @diehlpk 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 @shahmoradi

📝 Checklist for @gchure

diehlpk commented 1 year ago

Hi @stefanradev93 do you have time to review this paper?

gchure commented 1 year ago

Hi @gchure do you have time to review this paper?

Hi @diehlpk, I can review. This will be my first review with JOSS, so I would appreciate it if you could point me in the direction of some best practices.

shahmoradi commented 1 year ago

Thank you for the revision @dokempf. While I realize the majority of testing gaps are in a specific file, the authors should strive to bring code coverage above 90% across the whole project. @diehlpk, thank you for your reminder. The review of this paper is complete from my perspective.

diehlpk commented 1 year ago

@editorialbot add @gchure as reviewer

editorialbot commented 1 year ago

@gchure added to the reviewers list!

diehlpk commented 1 year ago

@gchure please have a look at the guidelines here

https://joss.readthedocs.io/en/latest/review_criteria.html

I am happy to answer all questions.

gchure commented 1 year ago

Review checklist for @gchure

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

gchure commented 1 year ago

I'm opening issues on the parafields repository as I go along through my review. My comments on the paper text are listed in #141.

Note that my paper comments are non-blocking and I leave it up to the discretion of @dokempf.

gchure commented 1 year ago

Here is an open issue (#142) outlining a error in reproducing the example for parallel usage of parafields. This is a blocking issue that should resolved before formal publication.

gchure commented 1 year ago

Here is an open issue (#143) which highlights an unsupported option in interactive mode. This is non-blocking and is at @dokempf's discretion.

gchure commented 1 year ago

Here is an open issue (#144) which highlights non-tested code that is advertised as a feature of parafields. I personally think that judging testing over coverage percent is silly (which is why I'm fine with 80% coverage), but the fenicsx integration function seems to be entirely untested. I don't think this should be a blocking issue, but really should be addressed. I leave that up to @diehlpk's discretion.

This is my final review comment. Once these issues are closed, I'm happy to recommend it be formally published in JOSS.

diehlpk commented 1 year ago

Here is an open issue (#144) which highlights non-tested code that is advertised as a feature of parafields. I personally think that judging testing over coverage percent is silly (which is why I'm fine with 80% coverage), but the Fenicsx integration function seems to be entirely untested. I don't think this should be a blocking issue, but really should be addressed. I leave that up to @diehlpk's discretion.

This is my final review comment. Once these issues are closed, I'm happy to recommend it be formally published in JOSS.

@gchure and @dokempf yes, I think it is non-blocking but I would recommend mentioning that the Fenics integration is untested in the documentation.

diehlpk commented 12 months ago

@dokempf can you please have a look at the comment above?

dokempf commented 11 months ago

@editorialbot generate pdf

editorialbot commented 11 months ago

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

dokempf commented 11 months ago

@editorialbot generate pdf

editorialbot commented 11 months ago

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

dokempf commented 11 months ago

@diehlpk We have addressed all review comments and are good to proceed. Thanks to @gchure and @shahmoradi for the work they put into this. Regarding the testing coverage of the FeniCSx integration, we went with documenting its state.

diehlpk commented 11 months ago

@gchure could you please have a final look amd check the two last missing boxes?

gchure commented 11 months ago

All of my issues have been addressed. Happy to close out my review. Congrats to @dokempf and the rest of the parafields contributors.

diehlpk commented 11 months ago

@shahmoradi and @gchure thanks for your helpful comments during the review. I will handle the submission now as editor.

diehlpk commented 11 months ago

@editorialbot check references

diehlpk commented 11 months ago

@editorialbot generate paper

editorialbot commented 11 months ago

I'm sorry human, I don't understand that. You can see what commands I support by typing:

@editorialbot commands

editorialbot commented 11 months ago
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1016/j.advwatres.2016.12.006 is OK
- 10.1137/s1064827592240555 is OK
- 10.1137/17m1149730 is OK
- 10.1109/MCSE.2021.3083216 is OK
- 10.1038/s41586-020-2649-2 is OK
- 10.1016/j.camwa.2020.06.007 is OK
- 10.5281/zenodo.2565368 is OK
- 10.2307/1390903 is OK
- 10.18637/jss.v055.i09 is OK
- 10.21105/joss.05595 is OK
- 10.5194/gmd-15-3161-2022 is OK

MISSING DOIs

- 10.1007/s10182-012-0196-3 may be a valid DOI for title: Spatio-temporal modeling of particulate matter concentration through the SPDE approach
- 10.1214/10-aoas369 may be a valid DOI for title: A spatial analysis of multivariate output from regional climate models
- 10.1137/130915005 may be a valid DOI for title: A hierarchical multilevel Markov chain Monte Carlo algorithm with applications to uncertainty quantification in subsurface flow
- 10.1016/j.neuroimage.2004.08.034 may be a valid DOI for title: Bayesian fMRI time series analysis with spatial priors
- 10.1115/1.1483342 may be a valid DOI for title: Random heterogeneous materials: microstructure and macroscopic properties
- 10.1137/16m1061692 may be a valid DOI for title: Quasi-Monte Carlo and multilevel Monte Carlo methods for computing posterior expectations in elliptic inverse problems

INVALID DOIs

- None
diehlpk commented 11 months ago

@editorialbot generate pdf

editorialbot commented 11 months ago

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

diehlpk commented 11 months ago

@dokempf I read the paper and request the following editorial changes:

Please fix these and we can proceed with the publication preparation.

dokempf commented 11 months ago

@editorialbot generate pdf

editorialbot commented 11 months ago

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

dokempf commented 11 months ago

@diehlpk I addressed your comments. Thanks for having a look.

diehlpk commented 11 months ago

@editorialbot generate pdf

editorialbot commented 11 months ago

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

diehlpk commented 11 months ago

@dokempf Thanks, it looks good now.

Please do the following:

dokempf commented 11 months ago

@diehlpk All done, The DOI is 10.5281/zenodo.10391370

diehlpk commented 11 months ago

@diehlpk All done, The DOI is 10.5281/zenodo.10391370

And the version of the release?

diehlpk commented 11 months ago

The title on Zenodo seems to be wrong

parafields/parafields: parafields v1.0.1

There should be the same title as on the paper.

diehlpk commented 11 months ago

@editorialbot set v1.0.1 as version

editorialbot commented 11 months ago

Done! version is now v1.0.1

diehlpk commented 11 months ago

@diehlpk All done, The DOI is 10.5281/zenodo.10391370

And the version of the release?

Nevermind, I found the version in the wrong Zenodo title.

dokempf commented 11 months ago

Oh, actually our paper is lacking a title. Funny how that went completely unnoticed so far.

diehlpk commented 11 months ago

@editorialbot generate pdf

diehlpk commented 11 months ago

Oh, the title should be parafields: A generator for distributed, stationary Gaussian processes or?

Please add that to the paper and archive.

editorialbot commented 11 months ago

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

dokempf commented 11 months ago

@diehlpk Finished, here is the DOI: 10.5281/zenodo.10355636

diehlpk commented 11 months ago

@editorialbot set 10.5281/zenodo.10355636 as archive

editorialbot commented 11 months ago

Done! archive is now 10.5281/zenodo.10355636

diehlpk commented 11 months ago

@editorialbot generate pdf

editorialbot commented 11 months ago

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

diehlpk commented 11 months ago

@dokempf the pdf is missing the title.