search

openjournals / joss-reviews

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

[REVIEW]: Bempp-cl: A fast Python based just-in-time compiling boundary element library. #2879

Closed whedon closed 3 years ago

whedon commented 3 years ago

Submitting author: @mscroggs (Matthew Scroggs) Repository: https://github.com/bempp/bempp-cl Version: v0.2.3 Editor: @meg-simula Reviewer: @jamtrott, @ramisetti Archive: 10.5281/zenodo.4618621

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

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

@jamtrott & @ramisetti, 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 @meg-simula 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 @jamtrott

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @ramisetti

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. @jamtrott, @ramisetti 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

: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 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1016/j.parco.2011.09.001 is OK
- 10.1002/nme.2579 is OK
- 10.1109/TAP.1982.1142818 is OK
- 10.1007/BF01396415 is OK
- 10.1016/j.crma.2004.12.022 is OK
- 10.1016/j.camwa.2017.07.049 is OK
- 10.1145/2833157.2833162 is OK
- 10.1007/978-0-387-68805-3 is OK
- 10.1145/3368618 is OK
- 10.1145/2590830 is OK

MISSING DOIs

- 10.2307/3621632 may be a valid DOI for title: Strongly elliptic systems and boundary integral equations

INVALID DOIs

- None
meg-simula commented 3 years ago

@jamtrott @ramisetti Thanks again for agreeing to review this submission - I will check back in in 1-2 weeks time to see how matters are progressing. If there is anything in the meanwhile, please let me know.

ramisetti commented 3 years ago

@meg-simula I have a problem to review. I don't have access to the reviewer checklist page for this submission.

meg-simula commented 3 years ago

@ramisetti: I see. Hm. Did you try these points?

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

ramisetti commented 3 years ago

@meg-simula: I tried as you suggested but I still cannot edit the checklist page. Also I did accept the invitation

ramisetti commented 3 years ago

Dear @meg-simula,

As I am unable to edit the review checklist, I am providing my review here. Please read below for my review of the Bempp-cl: A fast Python based just-in-time compiling boundary element library package for the Journal of Open Source Software.

Conflict of interest

• I confirm that I have read the JOSS conflict of interest (COI) policy and that: I have no COIs with reviewing this work or that any perceived COIs have been waived by JOSS for the purpose of this review

Code of Conduct

• I confirm that I read and will adhere to the JOSS code of conduct.

General checks

• Repository: Is the source code for this software available at the repository url? Yes • License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license? The repository contains LICENSE.md file with the contents of MIT license. The authors should consider renaming it to either LICENSE or COPYING as required for JOSS. • Contribution and authorship: Has the submitting author (@mscroggs) made major contributions to the software? Does the full list of paper authors seem appropriate and complete? The submitting authors have made major contributions to the software and the list of the authors seem appropriate and complete as well. • Substantial scholarly effort: Does this submission meet the scope eligibility described in the JOSS guidelines. This submission meets the scope eligibility as per the JOSS guidelines and it is clearly visible in terms of the length of commit history, number of commits, LOC, and its potential to attract researchers in this domain.

Functionality

• Installation: Does installation proceed as outlined in the documentation? I tried installing the software following instructions for the Quick Install and Installing from source. Both the approaches works smoothly without any issues with Python-3.8 on Mac OS. I have neither tried running it using Docker nor with OpenCL as I have don’t have them. • Functionality: Have the functional claims of the software been confirmed? The documentation claims the software can import and export in a number of formats, including Gmsh and VTK but it is not clearly evident from the examples provided in the applications section, except the exporting of gmsh file in the second example. The authors should provide at least one example for each case. Similarly, the authors should provide examples on the CPU and GPU parallelisation. The documentation for the examples should place a phrase related to the functionality to help the users understand the purpose of the example. For example, the documentation for each example can mention a phrase about the functionality in brackets beside the link • Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.) The authors make no performance claims

Documentation

• A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is? The authors clearly state the type of problems that can be solved using the software. The authors may consider mentioning about the target audience of this software to make it more clear to the potential users • Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution. The documentation mentions the list of required software to install the software. The dependencies are required to be explicitly installed by the user prior to installing the software. The software lacks the automated package management solution. The authors may consider mentioning the versions of the dependencies particularly for gmsh as this could save time for users in installing the dependencies • Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems). The documentation includes good number of examples on how to use the software. The authors should fix the broken link for “Weakly imposing a Dirichlet boundary condition” example. I would recommend to separate the tutorials from documentation as it is more intuitive to find examples in tutorials section rather in documentation • Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)? The python core functionality documentation is available through few links, but I believe it is not up to a satisfactory level to be useful for the prospective software developers interested in the software. The documentation should explicitly provide a link to API documentation • Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified? Although the repository contains a list of unit test but the documentation does not mention anything about any automated tests. The authors should consider updating the documentation with information on unit tests • 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: Information on third party contribution to the software is not provided. The documentation provides information on how to reporting issues or problems through GitHub repository. It also provides an email to seek any support related to the software

Software paper

• Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided? The paper tries to give a high-level functionality description through good references to BEM. I believe this will help non-specialist readers as well. • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is? The authors clearly state the type of problems that can be solved using the software. The authors may consider mentioning about the target audience of this software to make it more clear to readers • State of the field: Do the authors describe how this software compares to other commonly-used packages? This submission lacks information on why this software is better and how this compares to any of the existing open source software. The authors should consider including information in the paper if there are any existing other opensource software and how this software compares with them. • Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)? The paper is well written and structured as per the JOSS guidelines. However, there are few minor mistakes that should be corrected to make it better. i) “Calder’on” should be changed to “Calderón” in Section: Statement of need ii) “writing” should be changed to “written” in Section: Discretisation and solvers, iii) “fiels” should be changed to “field” in Section: Potential and far field operators, and iv) “in in” should be changed to “in” in Section: Further information • References: Is the list of references complete, and is everything cited appropriately that should be cited (e.g., papers, datasets, software)? Do references in the text use the proper citation syntax? I think the list of references is complete and everything is cited appropriately

meg-simula commented 3 years ago

Thanks @ramisetti for the prompt review! @mscroggs Are you able to handle the review comments in the format listed above?

meg-simula commented 3 years ago

@arfon Any ideas on why @ramisetti is not assigned to this issue (and not allowed to edit the checkboxes)? Anything I can do regarding this?

mscroggs commented 3 years ago

Yes, this format is fine. I'll get some revisions done soon

arfon commented 3 years ago

@whedon re-invite @ramisetti as reviewer

whedon commented 3 years ago

OK, the reviewer has been re-invited.

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

arfon commented 3 years ago

@arfon Any ideas on why @ramisetti is not assigned to this issue (and not allowed to edit the checkboxes)? Anything I can do regarding this?

@meg-simula - looks like something went awry with the repository invite. Can you try clicking the above link again @ramisetti? The invite should be shown at the top of this page: https://github.com/openjournals/joss-reviews/invitations

ramisetti commented 3 years ago

Thank you @arfon and @meg-simula. I can now edit the checkboxes.

meg-simula commented 3 years ago

@whedon re-invite @ramisetti as reviewer

Thanks @arfon. Next time this happens, should I just try this re-invite?

arfon commented 3 years ago

Thanks @arfon. Next time this happens, should I just try this re-invite?

Yes, that's it!

whedon commented 3 years ago

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

meg-simula commented 3 years ago

I think this was an automatic comment from @whedon, please feel free to disregard @ramisetti

meg-simula commented 3 years ago

@mscroggs Could you please give an update on the status of the review process?

mscroggs commented 3 years ago

@meg-simula We're waiting for me to finish making changes, they should be done shortly

mscroggs commented 3 years ago

@whedon generate pdf

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:

mscroggs commented 3 years ago

I've updated the paper following the reviewer comments. Notes on changes made can be found in this issue: https://github.com/bempp/bempp-cl/issues/94

mscroggs commented 3 years ago

@ramisetti @jamtrott Thanks for the reviews. I've just finished making changes to the paper and documentation (summary of changes at bempp/bempp-cl#94)

meg-simula commented 3 years ago

Thanks @mscroggs, well done!

So, it looks like all the boxes are checked, except one regarding State of the art. @ramisetti is this still an issue?

@jamtrott @ramisetti Are you satisfied with the revisions and changes made by @mscroggs, and have all your points been addressed?

meg-simula commented 3 years ago

/ooo February 3 to March 1

ramisetti commented 3 years ago

@meg-simula I am satisfied with the revisions made by @mscroggs. I believe the State of the art may provide a better comparison of the software to other existing ones. The authors may mention a phrase on how Bempp-cl is better/different from other any existing ones.

jamtrott commented 3 years ago

Hi!

Well done, @mscroggs. I'm happy with the changes, and I think the updated version is a nice improvement.

I don't have any further issues, but I will point out a couple of minor nitpicks.

In the section "Statement of need":

Once more, good work.

Sorry, it took a while for me to respond.

mscroggs commented 3 years ago

Thanks both, I'll get these sorted today

mscroggs commented 3 years ago

@whedon generate pdf

mscroggs commented 3 years ago

@jamtrott @ramisetti I've fixed those issues and expanded the statement of need a little. You can view a diff of the changes here: bempp/bempp-cl#105

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:

ramisetti commented 3 years ago

@meg-simula @mscroggs I am happy with the revised manuscript. I think all my points have been now addressed.

danielskatz commented 3 years ago

👋 @meg-simula - I think this is waiting on you at this point

meg-simula commented 3 years ago

@whedon generate pdf

meg-simula commented 3 years ago

@whedon check references

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 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1016/j.parco.2011.09.001 is OK
- 10.1002/nme.2579 is OK
- 10.1109/TAP.1982.1142818 is OK
- 10.1007/BF01396415 is OK
- 10.1016/j.crma.2004.12.022 is OK
- 10.1016/j.camwa.2017.07.049 is OK
- 10.1145/2833157.2833162 is OK
- 10.1007/978-0-387-68805-3 is OK
- 10.1145/3368618 is OK
- 10.1145/2590830 is OK
- 10.21105/joss.00043 is OK
- 10.1007/978-3-642-23099-8 is OK

MISSING DOIs

- 10.2307/3621632 may be a valid DOI for title: Strongly elliptic systems and boundary integral equations

INVALID DOIs

- None
meg-simula commented 3 years ago

Thanks again @jamtrott and @ramisetti for completing the reviews - much appreciated - and @mscroggs for addressing all comments. This looks good to go from my side. (The paper might be a bit long for the current JOSS standard, but I think the manuscript is valuable as is, and would recommend its current form.)

meg-simula commented 3 years ago

@mscroggs At this point could you:

I can then move forward with accepting the submission.

meg-simula commented 3 years ago

PS: whedon check references highlighted one possibly erraneous DOI in the references, would you @mscroggs check that prior to the above, please?

mscroggs commented 3 years ago

The missing DOI is for an old book that I couldn't find a DOI for. The suggested DOI is for a review of it, not the book itself. I'll sort those tasks out now

meg-simula commented 3 years ago

Ok, sounds good.

mscroggs commented 3 years ago

Tagged release: https://github.com/bempp/bempp-cl/releases/tag/v0.2.3 DOI of archive: https://dx.doi.org/10.5281/zenodo.4618621

mscroggs commented 3 years ago

@meg-simula: I think those tasks are all done now. Thanks for editing.

And thanks @jamtrott and @ramisetti for helpful reviewer comments.

meg-simula commented 3 years ago

@whedon set 10.5281/zenodo.4618621 as archive

whedon commented 3 years ago

OK. 10.5281/zenodo.4618621 is the archive.

meg-simula commented 3 years ago

@whedon set v0.2.3 as version

whedon commented 3 years ago

OK. v0.2.3 is the version.