openjournals / joss-reviews

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

[PRE REVIEW]: TSE: A triple stellar evolution code #7010

Closed editorialbot closed 3 weeks ago

editorialbot commented 1 month ago

Submitting author: !--author-handle-->@stegmaja<!--end-author-handle-- (Jakob Stegmann) Repository: https://github.com/stegmaja/TSE Branch with paper.md (empty if default branch): Version: v2.0 Editor: !--editor-->@warrickball<!--end-editor-- Reviewers: @rieder, @katiebreivik Managing EiC: Dan Foreman-Mackey

Status

status

Status badge code:

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

Author instructions

Thanks for submitting your paper to JOSS @stegmaja. Currently, there isn't a JOSS editor assigned to your paper.

@stegmaja if you have any suggestions for potential reviewers then please mention them here in this thread (without tagging them with an @). You can search the list of people that have already agreed to review and may be suitable for this submission.

Editor instructions

The JOSS submission bot @editorialbot is here to help you find and assign reviewers and start the main review. To find out what @editorialbot can do for you type:

@editorialbot commands
editorialbot commented 1 month ago

Hello human, 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 1 month ago

Software report:

github.com/AlDanial/cloc v 1.90  T=0.04 s (849.0 files/s, 266823.1 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Fortran 77                      21             22           2558           5473
Python                           7            585            742           1618
TeX                              1             17              0            278
Markdown                         3             77              0            270
C/C++ Header                     2              1              0            258
Bourne Shell                     3              4              2             17
make                             1              4              0             17
-------------------------------------------------------------------------------
SUM:                            38            710           3302           7931
-------------------------------------------------------------------------------

Commit count by author:

    85  Jakob Stegmann
    50  stegmaja
     5  Jordan Barber
editorialbot commented 1 month ago
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.48550/arXiv.2405.02912 is OK
- 10.1093/mnras/stac2192 is OK
- 10.1103/PhysRevD.106.023014 is OK
- 10.1093/mnras/stab287 is OK
- 10.3847/1538-4357/ab8461 is OK
- 10.1093/mnras/sty2848 is OK
- 10.1093/mnras/sty1999 is OK
- 10.3847/1538-4357/aacea4 is OK
- 10.3847/1538-4365/aa6fb6 is OK
- 10.1186/s40668-016-0019-0 is OK
- 10.1146/annurev-astro-081915-023315 is OK
- 10.1093/mnras/stu2396 is OK
- 10.1126/science.1223344 is OK
- 10.1046/j.1365-8711.2002.05038.x is OK
- 10.1046/j.1365-8711.2000.03426.x is OK

MISSING DOIs

- No DOI given, and none found for title: TRES: TRiple Evolution Simulation package
- No DOI given, and none found for title: MOBSE: Massive Objects in Binary Stellar Evolution

INVALID DOIs

- None
editorialbot commented 1 month ago

Paper file info:

šŸ“„ Wordcount for paper.md is 576

āœ… The paper includes a Statement of need section

editorialbot commented 1 month ago

License info:

āœ… License found: MIT License (Valid open source OSI approved license)

editorialbot commented 1 month ago

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

editorialbot commented 1 month ago

Five most similar historical JOSS papers:

COMPAS: A rapid binary population synthesis suite Submitting author: @ilyamandel Handling editor: @christinahedges (Retired) Reviewers: @katiebreivik, @HeloiseS Similarity score: 0.7399

Maelstrom: A Python package for identifying companions to pulsating stars from their light travel time variations Submitting author: @danhey Handling editor: @mbobra (Active) Reviewers: @hpparvi, @mbobra Similarity score: 0.7341

tomso: TOols for Models of Stars and their Oscillations Submitting author: @warrickball Handling editor: @christinahedges (Retired) Reviewers: @danhey, @astrobel Similarity score: 0.7283

LEGWORK: A python package for computing the evolution and detectability of stellar-origin gravitational-wave sources with space-based detectors Submitting author: @TomWagg Handling editor: @arfon (Active) Reviewers: @dbkeitel, @cmbiwer Similarity score: 0.7169

A-SLOTH: Ancient Stars and Local Observables by Tracing Halos Submitting author: @HartwigTilman Handling editor: @dfm (Active) Reviewers: @gregbryan, @kaleybrauer Similarity score: 0.7142

āš ļø Note to editors: If these papers look like they might be a good match, click through to the review issue for that paper and invite one or more of the authors before considering asking the reviewers of these papers to review again for JOSS.

dfm commented 1 month ago

@stegmaja ā€” Thanks for your submission! All the suitable JOSS editors are currently working at capacity so I'm going to "waitlist" this review until an editor with the relevant expertise is available to take it on. Thanks for your patience!

One thing I'd like to clarify off the bat: I think that the code in the mobse directory is not part of this review, right? I think that this submission just covers the Python code.

Also: while this submission waits in the queue, you might want to take a look at the JOSS review criteria especially the testing and installation/packaging. I think that these elements look like they could use some work before the review starts!

stegmaja commented 1 month ago

@dfm - Thank you very much! Indeed, mobse shall not be part of the review because it is essentially copied from the MOBSE gitlab repository (https://gitlab.com/mobse/source-code). I added an instalment instruction using the new yml file and will swiftly upload a test instruction.

warrickball commented 1 month ago

@editorialbot assign me as editor

editorialbot commented 1 month ago

Assigned! @warrickball is now the editor

warrickball commented 1 month ago

Hi @stegmaja, I'll start looking for reviewers. If there's anyone you have in mind who isn't a conflict of interest, let me know.

I'll also re-iterate @dfm's point above:

Also: while this submission waits in the queue, you might want to take a look at the JOSS review criteria especially the testing and installation/packaging. I think that these elements look like they could use some work before the review starts!

I just installed tse and, though it worked, several points already came up:

  1. The installation part of the README.md says to navigate to mobse to run make mobse. It should say "Navigate to the mobse/src directory." (as in compile.sh).
  2. It isn't clear why it's necessary to run the conda steps. I didn't and the program seemed to run correctly (though without tests I can't be sure).
  3. I couldn't find a description of the terminal output.
  4. Nothing mentioned that a CSV file would be created in the data folder, nor could I find a description of its contents.
warrickball commented 1 month ago

While I continue finding reviewers, we also need to resolve the open source status of MOBSE. I note that MOBSE is MIT-licensed, which appears consistent with TSE's licence. But MOBSE appears to be derived from (and indeed has a lot of identical code to) the original BSE, which as far as I can tell was (and still is) distributed without any licence. In it's current state, I don't think that part of the repo can be included in our review.

There are theoretically two ways to resolve this.

  1. The original author of BSE can be convinced to release it under an OSI-approved licence and, assuming that's the same version as used in MOBSE, MOBSE's licence can be made consistent.
  2. You can remove MOBSE from the RSE repo and include its download as part of the TSE installation instructions. I think this boils down to adding git clone https://gitlab.com/mobse/source-code mobse before the existing instructions.

The second option is reasonably straightforward and much more feasible than the first.

stegmaja commented 4 weeks ago

Hi @stegmaja, I'll start looking for reviewers. If there's anyone you have in mind who isn't a conflict of interest, let me know.

I'll also re-iterate @dfm's point above:

Also: while this submission waits in the queue, you might want to take a look at the JOSS review criteria especially the testing and installation/packaging. I think that these elements look like they could use some work before the review starts!

I just installed tse and, though it worked, several points already came up:

  1. The installation part of the README.md says to navigate to mobse to run make mobse. It should say "Navigate to the mobse/src directory." (as in compile.sh).
  2. It isn't clear why it's necessary to run the conda steps. I didn't and the program seemed to run correctly (though without tests I can't be sure).
  3. I couldn't find a description of the terminal output.
  4. Nothing mentioned that a CSV file would be created in the data folder, nor could I find a description of its contents.

Dear @warrickball,

thank you very much for serving as an editor to my code! Regarding your first points:

  1. I have changed README.md like you suggested.
  2. Theconda steps are to ensure that the user has all the required packages installed. While most of them are indeed standard and already installed for most users, I don't think that is the case for the gala package needed to (optionally) evolve the triples inside a Galactic potential.

I will add a description of 3. and 4. shortly.

Regarding your second comment about MOBSE: There are two minor changes I did to the mobse package in mobse/src/evolve.f and mobse/src/mobse.f to allow compatibility between my code and MOBSE, without actually changing the physics. Now I also highlight this in the README.md file. From that perspective would you think it is the most straightforward and user-friendly approach to only keep those two modified files in the repo but adding the rest of MOBSE with git clone ... like you suggested?

Best regards

Jakob

stegmaja commented 3 weeks ago

Edit: 3. and 4. are also added.

warrickball commented 3 weeks ago

Thanks for addressing these early issues. I've found two reviewers, so I'll start the proper review shortly but make a few remarks here.

Regarding your second comment about MOBSE: There are two minor changes I did to the mobse package in mobse/src/evolve.f and mobse/src/mobse.f to allow compatibility between my code and MOBSE, without actually changing the physics. Now I also highlight this in the README.md file. From that perspective would you think it is the most straightforward and user-friendly approach to only keep those two modified files in the repo but adding the rest of MOBSE with git clone ... like you suggested?

One possibility is to include a patch file and instructions on how to apply it when installing it. You could possibly even bundle a script to download, patch and install MOBSE in the appopriate place.

To be more concrete, here's how I started a patch file that I think has the desired effect.

$ git clone https://github.com/stegmaja/TSE
$ git clone git@gitlab.com:mobse/source-code.git mobse
$ cd TSE
$ diff -ruN ../mobse/src mobse/src > test.patch
$ cd ../mobse
$ patch -p1 < ../TSE/test.patch
patching file src/evolve.f
patching file src/mobse.f

So you could include the equivalent of test.patch in this repo and apply it as part of the installation. It turns out you can also do something similar with git. In particular, replacing the last step with

$ git apply ../TSE/test.patch

worked for me.

While experimenting, I also noticed that mobse/input/const_mobse.h also differ. I presume that doesn't matter.

The conda steps are to ensure that the user has all the required packages installed.

That's fair though bear in mind that not all Python users have Conda, and the implication is that TSE requires Conda, which doesn't seem to be the case.

warrickball commented 3 weeks ago

@editorialbot add @rieder as reviewer

editorialbot commented 3 weeks ago

@rieder added to the reviewers list!

warrickball commented 3 weeks ago

@editorialbot add @katiebreivik as reviewer

editorialbot commented 3 weeks ago

@katiebreivik added to the reviewers list!

warrickball commented 3 weeks ago

@editorialbot start review

editorialbot commented 3 weeks ago

OK, I've started the review over in https://github.com/openjournals/joss-reviews/issues/7102.