search

openjournals / joss-reviews

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

[REVIEW]: DisCoTec: Distributed higher-dimensional HPC simulations with the sparse grid combination technique #7018

Open editorialbot opened 4 months ago

editorialbot commented 4 months ago

Submitting author: !--author-handle-->@freifrauvonbleifrei<!--end-author-handle-- (Theresa Pollinger) Repository: https://github.com/SGpp/DisCoTec/ Branch with paper.md (empty if default branch): feature_joss_submission Version: 1.0.0 Editor: !--editor-->@danielskatz<!--end-editor-- 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 commented 4 months 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 4 months ago 
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 commented 4 months ago

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 commented 4 months ago

Paper file info:

📄 Wordcount for paper.md is 1044

✅ The paper includes a Statement of need section

editorialbot commented 4 months ago

License info:

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

EmilyBourne commented 4 months ago

Review checklist for @EmilyBourne

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

editorialbot commented 4 months ago

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

danielskatz commented 4 months ago

@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 commented 3 months ago

@EmilyBourne - how is your review coming along?

danielskatz commented 3 months ago

👋 @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 commented 3 months ago

@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 commented 3 months ago

@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 commented 3 months ago

Review checklist for @jakelangham

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

danielskatz commented 3 months ago

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

danielskatz commented 3 months ago

👋 @jakelangham - how is your review coming along?

jakelangham commented 3 months ago

@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 commented 3 months ago

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

freifrauvonbleifrei commented 3 months ago

very fair point about https://github.com/SGpp/DisCoTec/issues/129 . It may take a few weeks for us to get the documentation to a shareable state, I'll ping you when it's ready @jakelangham !

danielskatz commented 2 months ago

@freifrauvonbleifrei - how are you doing on responding to issues opened by the reviewers?

freifrauvonbleifrei commented 2 months ago

@danielskatz what a timely moment to ask :) The issue with the biggest impact was on feature documentation -- @obersteiner , @vancraar and me are currently putting the finishing touches on the new documentation at https://discotec.readthedocs.io/ !

At the same time, the main README is getting significantly shorter, see here: https://github.com/SGpp/DisCoTec/tree/feature_documentation .

@jakelangham maybe this tutorial helps achieve

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

? Let's continue this discussion in https://github.com/SGpp/DisCoTec/issues/129 :)

jakelangham commented 2 months ago

@freifrauvonbleifrei Fantastic - I'm away this week, so will take a proper look when it's fully done. At first glance it looks like exactly what we're after.

EmilyBourne commented 2 months ago

I am also on holiday this week and will only be able to look in detail next week. But I agree that the first look is promising

danielskatz commented 2 months ago

👋 @EmilyBourne & @jakelangham - as it's midway through a new week, I wanted to check with you again about your progress on this... thanks!

jakelangham commented 2 months ago

@danielskatz I've made some incremental progress on various items and can take another look in a day or 2.

jakelangham commented 2 months ago

@danielskatz @freifrauvonbleifrei I've finished my initial run through. There are a number of open issues left to address but after that we'll be on the home stretch.

freifrauvonbleifrei commented 2 months ago

organisatorial thing: I am going to be on vacation for two weeks starting tomorrow -- please don't expect a lot of immediate movement :) @jakelangham 's comments are gratefully received!

EmilyBourne commented 2 months ago

@editorialbot generate pdf

editorialbot commented 2 months ago

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

danielskatz commented 1 month ago

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

danielskatz commented 1 month ago

👋 @EmilyBourne - Can you remind me where things are for you? What is preventing you from making further progress in your review?

jakelangham commented 1 month ago

👋 @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 commented 1 month ago

@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 https://github.com/SGpp/DisCoTec/issues/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 https://github.com/SGpp/DisCoTec/pull/133 to be merged before checking the associated boxes in my review.

danielskatz commented 1 month ago

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

freifrauvonbleifrei commented 1 month ago

@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 https://github.com/SGpp/DisCoTec/pull/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 commented 1 month ago

@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 commented 1 month ago

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

EmilyBourne commented 1 month ago

No this all sounds good to me

danielskatz commented 1 month ago

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

freifrauvonbleifrei commented 1 month ago

Sorry, I had forgotten to push my added documentation to here: https://github.com/SGpp/DisCoTec/pull/133

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

danielskatz commented 1 month ago

Thanks.

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

danielskatz commented 2 weeks ago

@freifrauvonbleifrei - How is this going now?

freifrauvonbleifrei commented 2 weeks ago

@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: https://github.com/SGpp/DisCoTec/issues/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 commented 2 weeks ago

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 commented 1 week ago

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