search

openjournals / joss-reviews

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

[REVIEW]: kuibit: Analyzing Einstein Toolkit simulations with Python #3099

Closed whedon closed 3 years ago

whedon commented 3 years ago

Submitting author: @sbozzolo (Gabriele Bozzola) Repository: https://github.com/Sbozzolo/kuibit/ Version: 1.0.0 Editor: @eloisabentivegna Reviewer: @yurlungur, @eloisabentivegna Archive: 10.5281/zenodo.4681119

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

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

@yurlungur & @eloisabentivegna, 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 @eloisabentivegna 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 @yurlungur

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @eloisabentivegna

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. @yurlungur, @eloisabentivegna 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

PDF failed to compile for issue #3099 with the following error:

Can't find any papers to compile :-(

whedon commented 3 years ago 
Software report (experimental):

github.com/AlDanial/cloc v 1.88  T=0.68 s (130.3 files/s, 36408.4 lines/s)
--------------------------------------------------------------------------------
Language                      files          blank        comment           code
--------------------------------------------------------------------------------
Python                           37           4153           7062           8399
reStructuredText                 28            587            442           1449
Jupyter Notebook                  6              0           1518            274
Markdown                          4             62              0            210
Bourne Again Shell                6             30             54             93
INI                               3              3              0             79
YAML                              1             13              3             65
TOML                              2             13              3             51
make                              1              4              7              9
--------------------------------------------------------------------------------
SUM:                             88           4865           9089          10629
--------------------------------------------------------------------------------

Statistical information for the repository '1adefaf928447035a5a9a48d' was
gathered on 2021/03/12.
The following historical commit information, by author, was found:

Author                     Commits    Insertions      Deletions    % of changes
Gabriele Bozzola               203         27319           7703           99.99
deepsource-autofix[b             1             0              2            0.01

Below are the number of rows from each author that have survived and are still
intact in the current revision:

Author                     Rows      Stability          Age       % in comments
Gabriele Bozzola          19614           71.8          4.6               19.14
Yurlungur commented 3 years ago

@Sbozzolo Overall the paper reads quite well! One thing I noticed is that there is not much comparison to other visualization and analysis tools. For example, how does kuibit compare to visit, yt, or even pycactus? It may also be enough to mention that many users of the Einstein Toolkit write their own visualization pipelines by hand. I have created an issue to discuss on your repo here.

Sbozzolo commented 3 years ago

Thanks, I added a paragraph that lists all the software officially recommended in this page https://docs.einsteintoolkit.org/et-docs/Analysis_and_post-processing.

The only other software that is comparable to kuibit is SimulationTools, but it requires Mathematica (non-free). Most of the other software for analysis appears to be outdated, unmaintained, and/or lacking documentation (including PyCactus). Tools like visit cannot be used to do quantitative analysis but only visualization. So, I think that kuibit is unique, especially in its mission of being a package accessible to newcomers (and thus, its emphasis on documentation, tutorials, examples, comments in the code, and in requiring no proprietary software).

Yurlungur commented 3 years ago

Thanks, @Sbozzolo Yes, that looks great.

Yurlungur commented 3 years ago

Your contribution rules also look quite good. I added an issue with one question I had about them here.

Sbozzolo commented 3 years ago

Thanks, I updated the CONTRIBUTING document with a detailed list of the steps to contribute.

Yurlungur commented 3 years ago

Perfect!

Sbozzolo commented 3 years ago

@Yurlungur, @eloisabentivegna

I don't want to put any pressure on you, and I fully understand that JOSS is operating in "reduced service mode", but I wanted to let you know that by April 14th we will have to submit a end-of-year report for funding/computational time. We would love to include this project as part of that report, so it would be great if the paper was published before then. In case it won't, I will submit it to arXiv and use the arXiv number, so it is not a big deal (but I am sure you will understand that a published paper is better).

Please, don't take this message as a "request to do your review quickly". I know you are volunteering your time, and I fully respect that. And of course, I don't want the review to be rushed, as the process will improve kuibit. I just wanted you to know that if this paper will be published in the next three weeks, that would help our group.

Thanks!

Yurlungur commented 3 years ago

@Sbozzolo thanks for letting me know. I anticipate getting done well before that time, though I don't know how quickly JOSS publications appear after review.

eloisabentivegna commented 3 years ago

@Sbozzolo, thanks for bringing this up. I also think the review stage should complete well before your deadline, and the remaining stages are usually quite lightweight.

eloisabentivegna commented 3 years ago

@whedon generate pdf from branch joss-paper

whedon commented 3 years ago 
Attempting PDF compilation from custom branch joss-paper. Reticulating splines etc...
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

:wave: @eloisabentivegna, please update us on how your review is going (this is an automated reminder).

whedon commented 3 years ago

:wave: @yurlungur, please update us on how your review is going (this is an automated reminder).

eloisabentivegna commented 3 years ago

@Sbozzolo, I am almost done with the code review.

I have a few suggestions regarding the paper text:

I am also not sure I understand the note

(This notebook is meant to be converted in Sphinx documentation and not used directly.)

in the tutorial notebooks. Are these really not supposed to be used as tutorials? If not, what other document can users refer to for real-world examples?

eloisabentivegna commented 3 years ago

@Yurlungur, could you post an update?

Sbozzolo commented 3 years ago

@whedon generate pdf from branch joss-paper

whedon commented 3 years ago 
Attempting PDF compilation from custom branch joss-paper. Reticulating splines etc...
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:

Sbozzolo commented 3 years ago

Thanks, I took care of your comments. I added a sentence to say that kuibit does not assume any specific simulation workflow but gathers information from filenames and metadata.

I am also not sure I understand the note

(This notebook is meant to be converted in Sphinx documentation and not used directly.)

in the tutorial notebooks. Are these really not supposed to be used as tutorials?

The documentation notebooks are generated as part of the continuous integration pipeline using data that I include in the repo. They serve the purpose of showing the basic features available and how they should be used, but, due to technical limitations, the data included is very minimal. There is little point in running the notebooks interactively, as some of the features that are not used in the notebook will fail, given that the data is not realistic. If a user were to try to explore these other features, they would probably end up confused. In practice, I used jupyter only as a way to produce documentation that mixes code, comments, and output.

One can find real-world code looking at the experimental branch, which contains a large number of examples. The reason these are in the experimental branch is because all these scripts rely on at least one additional modules, argparse_helper, which I want to extend further before merging it back to master. This module is used to work with command-line arguments, and does not affect much the rest of the code, so the files are very good examples to learn how to work with kuibit. They are as real-world as it gets: if one is using the experimental branch, they can be used directly to do science. One can find this piece of information in the documentation. The readme of the repo also tries to make users aware of the experimental branch.

Since you couldn't find the examples, I guess I should make it more explicit.

Yurlungur commented 3 years ago

@Yurlungur, could you post an update?

My apologies---I fell ill last week and am only now catching up. I have much of the code review still to do. I can commit to finishing it by the end of the week.

eloisabentivegna commented 3 years ago

My apologies---I fell ill last week and am only now catching up. I have much of the code review still to do. I can commit to finishing it by the end of the week.

Thanks, @Yurlungur!

eloisabentivegna commented 3 years ago

Thanks, I took care of your comments. I added a sentence to say that kuibit does not assume any specific simulation workflow but gathers information from filenames and metadata.

Thanks!

Since you couldn't find the examples, I guess I should make it more explicit.

I agree. It's nice that there are pointers to the experimental branch and that users are told there are real-world examples there, but this is easy to miss at the moment. I suggest changing the section header in the readme from "Experimental Branch" to "Tutorials" (or something like that), and perhaps adding to the note in the docs/tutorials something like "For real-world tutorials, please see ...".

What do you think?

Sbozzolo commented 3 years ago

I agree. It's nice that there are pointers to the experimental branch and that users are told there are real-world examples there, but this is easy to miss at the moment. I suggest changing the section header in the readme from "Experimental Branch" to "Tutorials" (or something like that), and perhaps adding to the note in the docs/tutorials something like "For real-world tutorials, please see ...".

What do you think?

I added links to the examples in more places (e.g., the help section, at the bottom of the tutorials, in the opening of the readme), and expanded the section in the readme. (commit https://github.com/Sbozzolo/kuibit/commit/858bdf9e5f0526d72db5ba393ac393607131209b). Hopefully this helps.

Yurlungur commented 3 years ago

@Sbozzolo I think I've finished my review. I found a few very minor things. Mainly, when installing from poetry for development, I got hung up on a couple related details. The installation appears to be a virtualenv in the source directory, so I need to call poetry shell from within that directory, and python imports only work from there. I submit a PR to update the README here and a further question about the poetry equivalent of setup.py develop here

I also had some idle thoughts/questions, which I submitted as issues. They are not blocking and not really related to the review.

Other than that, I'm pretty happy with everything. I managed to use your documentation/tutorials to visualize a couple simple examples I had lying around. A BHNS merger, a TOV star, and the GW150914 gallery example from the ET website. All of it worked painlessly, thanks to the documentation and the inline docstrings. Overall, very impressive.

Sbozzolo commented 3 years ago

Thank you very much for your comments, @Yurlungur!

As for poetry. When you are in a poetry shell, the imports should work anywhere. When you are not in a poetry shell, the imports will work only in the source directory.

I realize that people might not be familiar with poetry, and I will definitely add more detailed instruction if I see that this is a point of struggle for (prospective) contributors.

Yurlungur commented 3 years ago

Hmm. I think I tried importing kuibit from within the poetry shell but in a different directory and it didn't work. But maybe I'm mistaken.

Sbozzolo commented 3 years ago

It is true that you have to run poetry shell from the directory where the code lives, but, once you have done it, you should be able to access kuibit from anywhere. poetry shell essentially activates a virtualenv.

Yurlungur commented 3 years ago

Got it. I must have made a mistake when I tried before.

eloisabentivegna commented 3 years ago

I am through with my review as well. The package is a very nice submission and, judging from the number of people I've heard over the years longing for a comprehensive viz package in Python for the ET, it fills a big gap. Thanks @Sbozzolo for the submission! I recommend acceptance.

The only reservation I have, perhaps along some of the things that @Yurlungur has expressed, is that poetry is a bit of a pain point. This would be OK if only people wanting to develop the package had the necessity to use it (although that means adding a barrier to contributions), but at the moment any user wanting to access the examples has to learn to use poetry. My recommendation is to merge the experimental features required by the examples into the main branch as soon as possible, so that standard Python installation routes become available.

eloisabentivegna commented 3 years ago

(Changing hat.)

@Sbozzolo: congratulation on the accepted submission! 🎉

Can you provide a tagged release and code archive details (see https://joss.readthedocs.io/en/latest/submitting.html#the-review-process)?

eloisabentivegna commented 3 years ago

@whedon check references from branch joss-paper

whedon commented 3 years ago 
Attempting to check references... from custom branch joss-paper
whedon commented 3 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.5281/zenodo.3866075 is OK
- doi:10.1088/0264-9381/29/11/115001 is OK
- 10.1103/PhysRevD.72.024028 is OK
- 10.1088/1361-6382/aa9cad is OK
- 10.1145/3311790.3396656 is OK

MISSING DOIs

- 10.1007/3-540-36569-9_13 may be a valid DOI for title: The Cactus Framework and Toolkit: Design and Applications

INVALID DOIs

- None
eloisabentivegna commented 3 years ago

@sbozzolo: could you also take care of the missing DOI before creating the tagged release and archiving the code?

Sbozzolo commented 3 years ago

Thanks, @eloisabentivegna. I'll address your comment about poetry, I'll fix the DOI, and I'll follow your instruction in the next two hours or so.

Sbozzolo commented 3 years ago

@whedon check references from branch joss-paper

whedon commented 3 years ago 
Attempting to check references... from custom branch joss-paper
whedon commented 3 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.5281/zenodo.3866075 is OK
- 10.1007/3-540-36569-9_13 is OK
- doi:10.1088/0264-9381/29/11/115001 is OK
- 10.1103/PhysRevD.72.024028 is OK
- 10.1088/1361-6382/aa9cad is OK
- 10.1145/3311790.3396656 is OK

MISSING DOIs

- None

INVALID DOIs

- None
Sbozzolo commented 3 years ago

The only reservation I have, perhaps along some of the things that @Yurlungur has expressed, is that poetry is a bit of a pain point. This would be OK if only people wanting to develop the package had the necessity to use it (although that means adding a barrier to contributions), but at the moment any user wanting to access the examples has to learn to use poetry. My recommendation is to merge the experimental features required by the examples into the main branch as soon as possible, so that standard Python installation routes become available.

My intent is definitely to require poetry only for developers. I understand that this may be a barrier, but I think that poetry is getting a lot of traction in the Python community (and alternatives equivalent to poetry would still require contributors to learn a new tool). Depending on the developer's feedback, I will be happy to provide further instruction on how to use poetry. However, given the already considerable amount of information and tutorials available online, my personal point of view is to treat poetry like any other tool in the Python ecosystem (e.g., unittest).

but at the moment any user wanting to access the examples has to learn to use poetry.

That's true if you want to run the examples, but you can still read them (and the comments) online.

so that standard Python installation routes become available.

You can install the experimental branch with:

pip3 install git+https://github.com/Sbozzolo/kuibit.git@experimental

My recommendation is to merge the experimental features required by the examples into the main branch as soon as possible,

At the moment, the development on the experimental branch is driven by my personal needs. I could merge some of the new features back to master, but not all the examples could be ported. So, I prefer waiting until things are in a great shape instead of merging work that I am not fully satisfied with. But, I 100% agree that this is a place where things can improved.

@Sbozzolo: could you also take care of the missing DOI before creating the tagged release and archiving the code?

I fixed that.

Can you provide a tagged release and code archive details (see https://joss.readthedocs.io/en/latest/submitting.html#the-review-process)?

I did! DOI: 10.5281/zenodo.4679863

Thanks!

eloisabentivegna commented 3 years ago

@whedon set 10.5281/zenodo.4679863 as archive

whedon commented 3 years ago

OK. 10.5281/zenodo.4679863 is the archive.

eloisabentivegna commented 3 years ago

@Sbozzolo, can I assume 1.0.0 is the tagged release you created for this submission?

Sbozzolo commented 3 years ago

Yes, correct.

On Mon, Apr 12, 2021, 00:09 Eloisa Bentivegna @.***> wrote:

@Sbozzolo https://github.com/Sbozzolo, can I assume 1.0.0 is the tagged release you created for this submission?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/3099#issuecomment-817547595, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACF6E7MITC2H7RYG45C3P23TIKMDHANCNFSM4ZC4PZVQ .

eloisabentivegna commented 3 years ago

@whedon set 1.0.0 as version

whedon commented 3 years ago

OK. 1.0.0 is the version.

eloisabentivegna commented 3 years ago

@whedon generate pdf from branch joss-paper