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

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

Closed
editorialbot opened this issue Jul 18, 2024 · 22 comments
Closed

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

editorialbot opened this issue Jul 18, 2024 · 22 comments
Assignees
Labels
Fortran pre-review Python TeX Track: 1 (AASS) Astronomy, Astrophysics, and Space Sciences

Comments

@editorialbot
Copy link
Collaborator

editorialbot commented Jul 18, 2024

Submitting author: @stegmaja (Jakob Stegmann)
Repository: https://github.com/stegmaja/TSE
Branch with paper.md (empty if default branch):
Version: v2.0
Editor: @warrickball
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 editorialbot added pre-review Track: 1 (AASS) Astronomy, Astrophysics, and Space Sciences labels Jul 18, 2024
@editorialbot
Copy link
Collaborator Author

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

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

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

Paper file info:

📄 Wordcount for paper.md is 576

✅ The paper includes a Statement of need section

@editorialbot
Copy link
Collaborator Author

License info:

✅ License found: MIT License (Valid open source OSI approved license)

@editorialbot
Copy link
Collaborator Author

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

@editorialbot
Copy link
Collaborator Author

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

dfm commented Jul 18, 2024

@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!

@dfm dfm added the waitlisted Submissions in the JOSS backlog due to reduced service mode. label Jul 18, 2024
@stegmaja
Copy link

@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
Copy link

@editorialbot assign me as editor

@editorialbot
Copy link
Collaborator Author

Assigned! @warrickball is now the editor

@warrickball warrickball removed the waitlisted Submissions in the JOSS backlog due to reduced service mode. label Jul 31, 2024
@warrickball
Copy link

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

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

stegmaja commented Aug 8, 2024

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. The conda 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
Copy link

Edit: 3. and 4. are also added.

@warrickball
Copy link

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 [email protected]: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
Copy link

@editorialbot add @rieder as reviewer

@editorialbot
Copy link
Collaborator Author

@rieder added to the reviewers list!

@warrickball
Copy link

@editorialbot add @katiebreivik as reviewer

@editorialbot
Copy link
Collaborator Author

@katiebreivik added to the reviewers list!

@warrickball
Copy link

@editorialbot start review

@editorialbot
Copy link
Collaborator Author

OK, I've started the review over in #7102.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fortran pre-review Python TeX Track: 1 (AASS) Astronomy, Astrophysics, and Space Sciences
Projects
None yet
Development

No branches or pull requests

4 participants