Skip to content
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]: DisCoTec: Distributed higher-dimensional HPC simulations with the sparse grid combination technique #7018

Open
editorialbot opened this issue Jul 22, 2024 · 59 comments
Assignees
Labels
C++ Python review Shell Track: 7 (CSISM) Computer science, Information Science, and Mathematics

Comments

@editorialbot
Copy link
Collaborator

editorialbot commented Jul 22, 2024

Submitting author: @freifrauvonbleifrei (Theresa Pollinger)
Repository: https://github.com/SGpp/DisCoTec/
Branch with paper.md (empty if default branch): feature_joss_submission
Version: 1.0.0
Editor: @danielskatz
Reviewers: @EmilyBourne, @jakelangham
Archive: Pending

Status

status

Status badge code:

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

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

@EmilyBourne & @jakelangham, 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 @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 @EmilyBourne

📝 Checklist for @jakelangham

@editorialbot
Copy link
Collaborator Author

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
Copy link
Collaborator Author

Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.18419/opus-9893 is OK
- 10.1145/3581784.3607036 is OK
- 10.1016/j.jcp.2023.112338 is OK
- 10.18419/opus-14210 is OK
- 10.1145/2807591.2807623 is OK

MISSING DOIs

- No DOI given, and none found for title: A Combination Technique for the Solution of Sparse...
- No DOI given, and none found for title: A spatially adaptive and massively parallel implem...

INVALID DOIs

- None

@editorialbot
Copy link
Collaborator Author

Software report:

github.com/AlDanial/cloc v 1.90  T=0.23 s (1260.5 files/s, 385341.1 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
JSON                             7              0              0          23826
C++                            114           3521           3766          19214
C/C++ Header                    85           3171           3342          12623
SVG                              4              4              4          11723
Python                          24            742            477           2931
Bourne Shell                    30            195            252            945
CMake                           15            109             70            453
Markdown                         5            121              0            435
Groovy                           1              4              4            302
TeX                              1              6              0             80
YAML                             2              1              4             20
INI                              1              1              0              5
-------------------------------------------------------------------------------
SUM:                           289           7875           7919          72557
-------------------------------------------------------------------------------

Commit count by author:

  1761	Theresa Pollinger
   392	Marcel Hurler
   130	Alexander Van Craen
   100	Obersteiner
    86	Freifrau von Bleifrei
    71	Mario Heene
    48	Michael Obersteiner
    20	Marvin Dostal
    15	Daniel Pfister
    12	ge25duq
    11	Marcel Breyer
     8	Keerthi Gaddameedi
     7	Christoph Niethammer
     3	Johannes Rentrop
     1	ipvober
     1	keerthi
     1	stormcorrosion

@editorialbot
Copy link
Collaborator Author

Paper file info:

📄 Wordcount for paper.md is 1044

✅ The paper includes a Statement of need section

@editorialbot
Copy link
Collaborator Author

License info:

🟡 License found: GNU Lesser General Public License v3.0 (Check here for OSI approval)

@EmilyBourne
Copy link

EmilyBourne commented Jul 22, 2024

Review checklist for @EmilyBourne

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

General checks

  • Repository: Is the source code for this software available at the https://github.com/SGpp/DisCoTec/?
  • License: Does the repository contain a plain-text LICENSE or COPYING file with the contents of an OSI approved software license?
  • Contribution and authorship: Has the submitting author (@freifrauvonbleifrei) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?
  • Substantial scholarly effort: Does this submission meet the scope eligibility described in the JOSS guidelines
  • Data sharing: If the paper contains original data, data are accessible to the reviewers. If the paper contains no original data, please check this item.
  • Reproducibility: If the paper contains original results, results are entirely reproducible by reviewers. If the paper contains no original results, please check this item.
  • Human and animal research: If the paper contains original data research on humans subjects or animals, does it comply with JOSS's human participants research policy and/or animal research policy? If the paper contains no such data, please check this item.

Functionality

  • Installation: Does installation proceed as outlined in the documentation?
  • Functionality: Have the functional claims of the software been confirmed?
  • Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

  • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
  • Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
  • Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
  • Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?
  • 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

  • Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
  • A statement of need: Does the paper have a section titled 'Statement of need' that clearly states what problems the software is designed to solve, who the target audience is, and its relation to other work?
  • State of the field: Do the authors describe how this software compares to other commonly-used packages?
  • Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
  • 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?

@editorialbot
Copy link
Collaborator Author

👉📄 Download article proof 📄 View article proof on GitHub 📄 👈

@danielskatz
Copy link

@EmilyBourne and @jakelangham - Thanks for agreeing to review this submission.
This is the review thread for the paper. All of our communications will happen here from now on.

As you can see above, you each should use the command @editorialbot generate my checklist to create your review checklist. @editorialbot commands need to be the first thing in a new comment.

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 openjournals/joss-reviews#7018 so that a link is created to this thread (and I can keep an eye on what is happening). Please also feel free to comment and ask questions on this thread. In my experience, it is better to post comments/questions/suggestions as you come across them instead of waiting until you've reviewed the entire package.

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.

@danielskatz
Copy link

@EmilyBourne - how is your review coming along?

@danielskatz
Copy link

👋 @jakelangham - note that you need to use the command @editorialbot generate my checklist to create your review checklist, in order to get started. @editorialbot commands need to be the first thing in a new comment.

@EmilyBourne
Copy link

@danielskatz Sorry it's coming a little slowly. I wasn't too worried as Jake didn't seem to have started but I will try and get it done ASAP.
I keep trying to carve out some time to look at it but have been quite busy the last couple of weeks. I will keep trying this week but if I don't manage, all my colleagues are on holiday next week so I should definitely be able to look at it then

@danielskatz
Copy link

@EmilyBourne - this certainly isn't urgent, but I try to ping on these after a couple of weeks just to make sure things have started and continue to move at some non-zero speed 🙂

@jakelangham
Copy link

jakelangham commented Aug 10, 2024

Review checklist for @jakelangham

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

General checks

  • Repository: Is the source code for this software available at the https://github.com/SGpp/DisCoTec/?
  • License: Does the repository contain a plain-text LICENSE or COPYING file with the contents of an OSI approved software license?
  • Contribution and authorship: Has the submitting author (@freifrauvonbleifrei) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?
  • Substantial scholarly effort: Does this submission meet the scope eligibility described in the JOSS guidelines
  • Data sharing: If the paper contains original data, data are accessible to the reviewers. If the paper contains no original data, please check this item.
  • Reproducibility: If the paper contains original results, results are entirely reproducible by reviewers. If the paper contains no original results, please check this item.
  • Human and animal research: If the paper contains original data research on humans subjects or animals, does it comply with JOSS's human participants research policy and/or animal research policy? If the paper contains no such data, please check this item.

Functionality

  • Installation: Does installation proceed as outlined in the documentation?
  • Functionality: Have the functional claims of the software been confirmed?
  • Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

  • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
  • Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
  • Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
  • Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?
  • 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

  • Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
  • A statement of need: Does the paper have a section titled 'Statement of need' that clearly states what problems the software is designed to solve, who the target audience is, and its relation to other work?
  • State of the field: Do the authors describe how this software compares to other commonly-used packages?
  • Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
  • 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?

@danielskatz
Copy link

@EmilyBourne - thanks for opening these issues
@freifrauvonbleifrei - thanks for some responses.

@danielskatz
Copy link

👋 @jakelangham - how is your review coming along?

@jakelangham
Copy link

@danielskatz, @freifrauvonbleifrei I took a proper look today. (Apologies if I'm late.) From what I've seen at so far, it looks like a well written and potentially useful library that seems entirely suitable for a JOSS paper.

However, at the outset I'd like to echo @EmilyBourne's observation in SGpp/DisCoTec#129 about the lack of documentation. The README is nice and very welcome but I was expecting to see some sort of detailed guide for a newcomer about how to actually use the software. Without this, I cannot adequately review the library because I only have a partial understanding of what it does and what all its features are. Therefore, I think that major revisions are required in this direction before proceeding further.

The JOSS reviewer guidelines are somewhat discretionary on what counts as sufficient documentation, but essentially what I'm looking for is to convince myself that a target user could get to grips with DisCoTec and use it to solve problems completely independently of input from the developers and I don't think this is currently the case.

@danielskatz
Copy link

Thanks @jakelangham - I think your comment is correct about what JOSS is looking for

that a target user could get to grips with DisCoTec and use it to solve problems completely independently of input from the developers

@jakelangham
Copy link

👋 @jakelangham - I think you are waiting for @freifrauvonbleifrei to address your comments, correct? (though @freifrauvonbleifrei is out for another week or so...)

That's correct, and they're not all trivial so imagine it may take some time to work through them all.

@EmilyBourne
Copy link

@danielskatz I am in a similar place to @jakelangham .

I was waiting for the documentation to be addressed so I could try to use the software on some of my own problems to ensure that the functionality is ok and that the documentation is clear enough to make this possible.
However for the moment I agree with @jakelangham 's comment on SGpp/DisCoTec#129

I was just about able to follow the tutorial, which I think is very valuable. Nevertheless, to understand the code snippets I often wanted to see more context on what some of the functions called are doing. This led me to click on the 'Library API', which is yet to exist. I hate to say it, but this could be useful. API documentation is listed as a requirement in the JOSS review criteria (https://joss.readthedocs.io/en/latest/review_criteria.html#api-documentation) and seems particularly important for a code such as this which is designed to work like a library that users build codes on top of.

Without a Library API it is difficult to have an overview of the functionalities. We can check the ones used in the tutorial but I imagine that these are not the only functions available.

I am also waiting for SGpp/DisCoTec#133 to be merged before checking the associated boxes in my review.

@danielskatz
Copy link

👋 @freifrauvonbleifrei - Can you update us on your progress (and/or schedule) for responding to the issues from @jakelangham and @EmilyBourne?

@freifrauvonbleifrei
Copy link

freifrauvonbleifrei commented Oct 22, 2024

@danielskatz thank you for raising the uncomfortable questions! ;)

Edit: The API Documentation is now linking correctly: https://discotec.readthedocs.io/en/latest/api/library_root.html
Easy fix.

The harder questions were arising for me once I started fine-grained documentation: I don't think I and the co-authors will be able to cover the full API in detail -- this is the curse of a multi-PhD-generational code (it's broad, and would need significant brainpower to re-understand every part). But I'd rather not throw out useful functionality. On the other hand, snippets of Doxygen are sprinkled throughout all parts of the code, making the current Doxygen overview quite messy to read.

So my suggestion would be to focus on the classes that framework users would most likely interact with (DistributedSparseGridUniform, ProcessGroupWorker, TaskWorker, SparseGridWorker, CombiParameters, CombiScheme, CombiCom, ProcessManager and ProcessGroupManager, in my opinion).
Once this is done (by the end of next week?), I would merge SGpp/DisCoTec#133 ; so that @EmilyBourne and @jakelangham could start on integrating it with their applications, and let me know in which parts more or less documentation depth is required.
(Concurrently, I would try to understand Doxygen sufficiently well to remove the "Library API" documentation parts that are obviously useless.)

Please do let me know your thoughts and inner resistances -- quite possibly, there is a smarter way to go about it.

@jakelangham
Copy link

@freifrauvonbleifrei I agree with this approach - it's best to focus on the core functionality. Even better if you can highlight the important parts of the API (or remove the unnecessary auto-generated boilerplate stuff). The review criteria for JOSS indicate that only the core functions/classes needed to be documented (though obviously more documentation is usually better).

@danielskatz
Copy link

Thanks @freifrauvonbleifrei and @jakelangham. Let's go ahead unless @EmilyBourne has a different suggestion that we should discuss first.

@EmilyBourne
Copy link

No this all sounds good to me

@danielskatz
Copy link

@freifrauvonbleifrei - I just want to confirm that you are working on this...

@freifrauvonbleifrei
Copy link

Sorry, I had forgotten to push my added documentation to here: SGpp/DisCoTec#133

I am currently trying to remove the worst documentation clutter and am on track to merge today :)

@danielskatz
Copy link

Thanks.

(and FYI, I may be off-line next week)

@danielskatz
Copy link

@freifrauvonbleifrei - How is this going now?

@freifrauvonbleifrei
Copy link

@danielskatz good question!

I was hoping that now @EmilyBourne and @jakelangham would start their hands-ons, while I further refine the documentation (progress here https://github.com/SGpp/DisCoTec/tree/refine_documentation) according to the things that were already mentioned here: SGpp/DisCoTec#129 , and trying to make the Doxygen output more usable.

probably issue 129 would also be a good spot for further docs-related feedback to give things to incorporate on a rolling basis ;)

@EmilyBourne
Copy link

I have a couple of deadlines coming up soon so I haven't got back to this yet but I will get on it as soon as I have time (hopefully some time next week)

@danielskatz
Copy link

👋 @EmilyBourne & @jakelangham - just checking with you on this again...

@EmilyBourne
Copy link

@danielskatz I am just getting back to this but am still quite busy. My last deadline this year is 15th December. I have started looking through the docs again but I'm not sure I will have the chance to test out my own implementations before then. After the 15th though I should have enough time so I will make sure I am done with my review by the 20th.

@danielskatz
Copy link

@jakelangham - it looks like you are close to done. Can you finish up fairly soon as well? Or are there things blocking you?

@jakelangham
Copy link

Hi, @danielskatz, have I read back through the thread and hope there hasn't been a miscommunication here. I finished my review (in the sense of interrogating every bullet point) some time ago and was waiting for the points I raised to be addressed, after which I expected to be happy to recommend for publication.

Specifically, I can see SGpp/DisCoTec#134, SGpp/DisCoTec#137, SGpp/DisCoTec#129. As far as I'm aware there has been some progress but they're still being worked on.

@danielskatz
Copy link

Thanks @jakelangham. It's helpful to have this in one place, and I apologize if it seemed like I was blaming you; I was just trying to pull all the statuses together.

👋 @freifrauvonbleifrei - can you update us here on how the work on these open issues are going?

@freifrauvonbleifrei
Copy link

@jakelangham thx for the overview! I had started working diffusely on the different questions but it really helps to see them as a (finite) list! :D
some feedback requests follow in the respective issues.

@freifrauvonbleifrei
Copy link

@danielskatz I think that now we are on top of all the issues, but happy to know if there's more we can clarify / extend in the documentation.
(I think I cannot keep exhale from generating the "Class Hierarchy" and "File Hierarchy" in the Library API, but also I have gotten kind of used to them by now ;) )

@freifrauvonbleifrei
Copy link

@editorialbot check references

@editorialbot
Copy link
Collaborator Author

Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

✅ OK DOIs

- 10.18419/opus-9893 is OK
- 10.1016/j.jcp.2023.112338 is OK
- 10.18419/opus-14210 is OK
- 10.1145/2807591.2807623 is OK
- 10.48550/arXiv.2203.09314 is OK
- 10.1016/j.jcp.2021.110495 is OK

🟡 SKIP DOIs

- No DOI given, and none found for title: A Combination Technique for the Solution of Sparse...
- No DOI given, and none found for title: Sparse Grids
- No DOI given, and none found for title: A spatially adaptive and massively parallel implem...
- No DOI given, and none found for title: Lorenzo-Tamellini/Sparse-Grids-Matlab-Kit
- No DOI given, and none found for title: DisCoTec-combischeme-utilities
- No DOI given, and none found for title: SGpp/SGpp
- No DOI given, and none found for title: sparseSpACE - The Sparse Grid Spatially Adaptive C...

❌ MISSING DOIs

- 10.1103/revmodphys.79.421 may be a valid DOI for title: Foundations of Nonlinear Gyrokinetic Theory
- 10.1088/0741-3335/47/5a/017 may be a valid DOI for title: Particle Simulation of Plasmas: Review and Advance...

❌ INVALID DOIs

- 10.1109/SC41406.2024.00104 is INVALID

@freifrauvonbleifrei
Copy link

@editorialbot check references

@editorialbot
Copy link
Collaborator Author

Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

✅ OK DOIs

- 10.18419/opus-9893 is OK
- 10.1016/j.jcp.2023.112338 is OK
- 10.18419/opus-14210 is OK
- 10.1145/2807591.2807623 is OK
- 10.48550/arXiv.2203.09314 is OK
- 10.1103/revmodphys.79.421 is OK
- 10.1016/j.jcp.2021.110495 is OK
- 10.1088/0741-3335/47/5a/017 is OK

🟡 SKIP DOIs

- No DOI given, and none found for title: A Combination Technique for the Solution of Sparse...
- No DOI given, and none found for title: Sparse Grids
- No DOI given, and none found for title: A spatially adaptive and massively parallel implem...
- No DOI given, and none found for title: Lorenzo-Tamellini/Sparse-Grids-Matlab-Kit
- No DOI given, and none found for title: DisCoTec-combischeme-utilities
- No DOI given, and none found for title: SGpp/SGpp
- No DOI given, and none found for title: sparseSpACE - The Sparse Grid Spatially Adaptive C...

❌ MISSING DOIs

- None

❌ INVALID DOIs

- 10.1109/SC41406.2024.00104 is INVALID

@danielskatz
Copy link

@danielskatz I think that now we are on top of all the issues, but happy to know if there's more we can clarify / extend in the documentation. (I think I cannot keep exhale from generating the "Class Hierarchy" and "File Hierarchy" in the Library API, but also I have gotten kind of used to them by now ;) )

Thanks - I hope the reviewers can comment on if more is needed.

@jakelangham
Copy link

Hi all, I am travelling (again) this week but should be able to get to review everything again before end-of-year holidays.

@jakelangham
Copy link

@freifrauvonbleifrei @danielskatz I've looked through all the improvements and submitted a few suggestions but they are very minor in scope, so I probably don't have more to add here unless there are specific questions about things. In short, I'd be happy if the paper proceeded forward to publication.

@freifrauvonbleifrei Two additional things that need doing beforehand: (1) check the references + DOIs (e.g. link on the last ref, [Verboncouer 2005] might need tidying up somehow); (2) all the improvements need merging into main (excepting the paper, which can live on its own branch, if preferred).

@danielskatz
Copy link

Thanks @jakelangham!

@danielskatz
Copy link

👋 @EmilyBourne - just a reminder on this, which you probably don't need...

@danielskatz I am just getting back to this but am still quite busy. My last deadline this year is 15th December. I have started looking through the docs again but I'm not sure I will have the chance to test out my own implementations before then. After the 15th though I should have enough time so I will make sure I am done with my review by the 20th.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C++ Python review Shell Track: 7 (CSISM) Computer science, Information Science, and Mathematics
Projects
None yet
Development

No branches or pull requests

5 participants