-
-
Notifications
You must be signed in to change notification settings - Fork 38
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[REVIEW]: ngsPETSc: A coupling between NETGEN/NGSolve 2 and PETSc #7359
Comments
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:
For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:
|
Software report:
Commit count by author:
|
|
Paper file info: 📄 Wordcount for ✅ The paper includes a |
License info: ✅ License found: |
Review checklist for @knepleyConflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
|
@thelfer & @knepley - Thanks for agreeing to review this submission. As you can see above, you each should use the command As you go over the submission, please check any items that you feel have been satisfied. There are also links to the JOSS reviewer guidelines. The JOSS review is different from most other journals. Our goal is to work with the authors to help them meet our criteria instead of merely passing judgment on the submission. As such, reviewers are encouraged to submit issues and pull requests on the software repository. When doing so, please mention We aim for reviews to be completed within about 2-4 weeks. Please let me know if either of you require some more time. We can also use editorialbot (our bot) to set automatic reminders if you know you'll be away for a known period of time. Please feel free to ping me (@danielskatz) if you have any questions/concerns. |
Review checklist for @thelferConflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
|
@danielskatz Looking at the repo, based on the number of commits, number of lines, and recently opened MRs, the authors should probably also include JDBetteridge in the author list. How do you normally approach this at JOSS? |
@knepley - This is really a question for the author, rather than for JOSS. 👋 @UZerbinati - can you say something about this? |
@danielskatz Oh, I meant "I am allowed to talk directly to the author", so I guess the answer is yes :) |
Yes! JOSS reviewers are meant to be interactive between the author(s) and reviewers, with my role being to keep things on track and progressing. Think of this like any other open source software discussion. |
@UZerbinati There is not really a State of the Field in the paper, which would be a brief list of packages with similar capabilities, maybe DUNE or FreeFEM? |
@UZerbinati I don't see guidelines for contributing. Perhaps it would be enough to point to the Firedrake guidelines in the documentation? |
Dear @danielskatz and @knepley, the reason why Jack is not among the authors is the fact that most of his contributions were made after the submission of this manuscript. After talking with the other authors and Jack, we decided to add Jack among the paper's authors. |
As suggested by @knepley in openjournals/joss-reviews#7359 we added contribution guidelines.
As suggested by @knepley in openjournals/joss-reviews#7359 we added contribution guidelines.
👋 @thelfer - Will you be able to start on your review soon? |
I'll start my review next week after my vacations :) |
👋 @thelfer - How are things going now? |
@danielskatz Slowly for sure. I made issues on the installation process.
I must now recompile |
I am pretty busy this week and the next one, but after that sky is blue, so I think that the review will be over at the end of the month |
@knepley - I think your concerns (or some of them) might have been addressed. Can you take another look and see if there's more that you can check off your review checklist? |
Thanks for the input. I'll have a look quickly. |
@danielskatz I almost finished my review. There are still a few questions left regarding the availability of the tests cases exhibited in the paper: NGSolve/ngsPETSc#64 |
Issue NGSolve/ngsPETSc#64 has been quickly fixed. On my side, everything is ok and the paper is ready to be published. |
Thanks @thelfer! |
@thelfer - can you check off the last item in your checklist? |
👋 @knepley - How are you doing on this? It looks like you just have a few items left to check off. I wonder if they have been addressed by the authors now. |
Which one ? I did not check for automated tests and did not see any "state of the art" section in the documentation. Are they required ? |
👋 @thelfer Note that the definition for Automated tests (which might be slightly misnamed) is "Are there automated tests or manual steps described so that the functionality of the software can be verified?" And yes, the paper is required to discuss the state of the field - "Do the authors describe how this software compares to other commonly-used packages?" If neither of these can be checked off by you currently, the authors will need to address them before we can say that your review is complete and the work can be published. |
Digging in the
I would say that this point is questionnable. The paper shows how useful it is for I don't even know if this criterion is applicable in this context. |
Regarding automated testing, we also have a CI which was running smoothly until yesterday. I'm planning to find out the bug that is causing its failure this weekend. Here is the GitHub workflow for the CI: https://github.com/NGSolve/ngsPETSc/blob/main/.github/workflows/ngsPETSc.yml. Regarding the state of the art, as suggested by @knepley we added a paragraph where we briefly list packages with similar capabilities, including DUNE and FreeFEM, here you can find the MR: NGSolve/ngsPETSc#62 |
@UZerbinati Thanks for the inputs |
Thanks again @thelfer |
My pleasure. |
Submitting author: @UZerbinati (Umberto Zerbinati)
Repository: https://github.com/NGSolve/ngsPETSc
Branch with paper.md (empty if default branch):
Version: v.0.0.5
Editor: @danielskatz
Reviewers: @thelfer, @knepley
Archive: Pending
Status
Status badge code:
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
@thelfer & @knepley, 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:
The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @danielskatz 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 @knepley
📝 Checklist for @thelfer
The text was updated successfully, but these errors were encountered: