search

openjournals / joss-reviews

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

[REVIEW]: SIMSOPT: A flexible framework for stellarator optimization #3525

Closed whedon closed 3 years ago

whedon commented 3 years ago

Submitting author: @landreman (Matt Landreman) Repository: https://github.com/hiddenSymmetries/simsopt Version: v0.4.7 Editor: @dfm Reviewer: @ZedThree, @StanczakDominik Archive: 10.5281/zenodo.5498111

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

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

@ZedThree & @StanczakDominik, 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 @dfm 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 @ZedThree

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @StanczakDominik

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. @ZedThree, @StanczakDominik 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
dfm commented 3 years ago

@ZedThree, @StanczakDominik, @landreman – This is the review thread for the paper. All of our communications will happen here from now on. Thanks again for agreeing to participate!

Please read the "Reviewer instructions & questions" in the first comment above.

Both reviewers have checklists at the top of this thread (in that first comment) with the JOSS requirements. 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, the reviewers are encouraged to submit issues and pull requests on the software repository. When doing so, please mention openjournals/joss-reviews#3525 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 the review process to be completed within about 4-6 weeks but please make a start well ahead of this as JOSS reviews are by their nature iterative and any early feedback you may be able to provide to the author will be very helpful in meeting this schedule.

dfm commented 3 years ago

@whedon generate pdf from branch joss_paper

☝️ Since the paper lives on the joss_paper branch in the repo, I think we'll need to use this command to compile the paper.

whedon commented 3 years ago 
Attempting PDF compilation from custom branch joss_paper. Reticulating splines etc...
danielskatz commented 3 years ago

@whedon commands

whedon commented 3 years ago

Here are some things you can ask me to do:

# List all of Whedon's capabilities
@whedon commands

# Assign a GitHub user as the sole reviewer of this submission
@whedon assign @username as reviewer

# Add a GitHub user to the reviewers of this submission
@whedon add @username as reviewer

# Re-invite a reviewer (if they can't update checklists)
@whedon re-invite @username as reviewer

# Remove a GitHub user from the reviewers of this submission
@whedon remove @username as reviewer

# List of editor GitHub usernames
@whedon list editors

# List of reviewers together with programming language preferences and domain expertise
@whedon list reviewers

# Change editorial assignment
@whedon assign @username as editor

# Set the software archive DOI at the top of the issue e.g.
@whedon set 10.0000/zenodo.00000 as archive

# Set the software version at the top of the issue e.g.
@whedon set v1.0.1 as version

# Open the review issue
@whedon start review

EDITORIAL TASKS

# All commands can be run on a non-default branch, to do this pass a custom 
# branch name by following the command with `from branch custom-branch-name`.
# For example:

# Compile the paper
@whedon generate pdf

# Compile the paper from alternative branch
@whedon generate pdf from branch custom-branch-name

# Remind an author or reviewer to return to a review after a
# certain period of time (supported units days and weeks)
@whedon remind @reviewer in 2 weeks

# Ask Whedon to do a dry run of accepting the paper and depositing with Crossref
@whedon recommend-accept

# Ask Whedon to check the references for missing DOIs
@whedon check references

# Ask Whedon to check repository statistics for the submitted software
@whedon check repository

EiC TASKS

# Invite an editor to edit a submission (sending them an email)
@whedon invite @editor as editor

# Reject a paper
@whedon reject

# Withdraw a paper
@whedon withdraw

# Ask Whedon to actually accept the paper and deposit with Crossref
@whedon accept deposit=true
danielskatz 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...
danielskatz 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 
Software report (experimental):

github.com/AlDanial/cloc v 1.88  T=0.27 s (840.7 files/s, 131387.4 lines/s)
--------------------------------------------------------------------------------
Language                      files          blank        comment           code
--------------------------------------------------------------------------------
Python                          114           3886           5991          14065
C++                              22            292             86           3000
C/C++ Header                     32            458            192           2847
reStructuredText                 21            510            407           1211
YAML                             11            188            190            761
SparForte                         5             12              0            629
Markdown                          5             59              1            172
Bourne Again Shell                9             49             82             88
INI                               1             10              0             81
CMake                             1             15             12             76
DOS Batch                         1              8              1             26
CSS                               2              2             12             24
make                              1              5              7             12
Bourne Shell                      1              0              2              3
TOML                              1              0              0              3
--------------------------------------------------------------------------------
SUM:                            227           5494           6983          22998
--------------------------------------------------------------------------------

Statistical information for the repository '95ef2adc3d80f9a1544fb250' was
gathered on 2021/07/22.
The following historical commit information, by author, was found:

Author                     Commits    Insertions      Deletions    % of changes
Andrew Giuliani                 84          6965           4537           11.80
Bharat Kumar Medasan             8           117            286            0.41
Bharat Medasani                128         13491           2650           16.56
Caoxiang Zhu                     9           281             40            0.33
Elizabeth                       25          2267            397            2.73
Florian Wechsung               256         24779          12397           38.14
Matt Landreman                 153         14899           8198           23.70
Rogerio Jorge                   70          3440           1672            5.24
Zhisong Qu                       1             5              3            0.01
ejpaul                           5           649            402            1.08

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
Andrew Giuliani            2332           33.5          3.0               13.16
Bharat Medasani            6167           45.7          3.8               17.42
Elizabeth                   954           42.1          1.6                7.13
Florian Wechsung          11254           45.4          2.8                4.70
Matt Landreman             7849           52.7          5.8               18.21
Rogerio Jorge              2262           65.8          2.2                9.37
Zhisong Qu                    2           40.0          6.1                0.00
whedon commented 3 years ago

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

 Can't find any papers to compile :-(
arfon 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:

dfm commented 3 years ago

@ZedThree, @StanczakDominik, @landreman — sorry about all the noise! Our bot @whedon was having issues, but it looks like we're back up and running.

StanczakDominik commented 3 years ago

Hey, I just wanted to report back and say that I'm sorry for the delay. I've had an intense few weeks. I'm now heading for my first vacation in a couple years (gee thanks, COVID!). Once I'm back next Tuesday, I'm making going through the rest of the package my first priority. The parts I've seen so far are looking great, by the way!

whedon commented 3 years ago

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

whedon commented 3 years ago

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

ZedThree commented 3 years ago

Mostly done, I need to dig a bit more into the functionality and API docs. But generally, everything's been of a high standard so far.

StanczakDominik commented 3 years ago

Mostly done with my review! Everything looks, well, stellar. 🌞

One concern I have is the inclusion/dependency on SPEC. Based on some of my digging around (and https://github.com/hiddenSymmetries/simsopt/issues/35#issuecomment-854786329 ), my understanding is that:

I have a few ideas about this, in order of increasing feasibility:

  1. Open source SPEC! This is of course both the ideal world scenario and out of your hands, and was apparently in-progress-but-will-take-a-while in April: https://github.com/hiddenSymmetries/simsopt/issues/35#issuecomment-815885445 ;
  2. Contribute the SPEC binary building process of the SIMSOPT docker build upstream, so SPEC can remain closed source but provide the binaries (probably a waste of time depending on how close SPEC is to becoming open source, though);
  3. Note more visibly (in the paper? in the SIMSOPT readme? maybe both?) that, as of {date}, SPEC is on the way to open source, but not there yet, and for now people can use the Docker image that provides its binaries. This kind of thing should be stated openly, just so people don't have to dig into this issue themselves.
    • Ideally, add some notes on the 3d viz out of Docker issue, which I'm not sure has been considered (I imagine SIMSOPT devs have SPEC access!).

Besides that, though, really a cool piece of software and I definitely hope I'll get to use it some day :)

landreman commented 3 years ago

@StanczakDominik Thanks for taking the time to look at SIMSOPT. We definitely recognize the issue about SPEC not being open-source yet, and have been working on it. SPEC was developed at a national lab so there is a lot of bureaucracy to go through for this. We checked in again today with the people at the lab, and all the paperwork has been submitted to US DOE to make SPEC open-source, but we have to wait for their final approval to make the repo public, and I don't know how long this will take. In the meantime we followed your option 3: I added text to the simsopt README, to the index page of the docs, and to the JOSS paper to say that we expect SPEC to be open-source soon but it's not there yet. We also appreciate the point about mayavi. We'll work to allow other rendering packages for those plot functions, either replacing mayavi or providing other choices.

StanczakDominik commented 3 years ago

Awesome! And I'll try to help out with the Mayavi replacement ASAP - probably this or next weekend :)

landreman commented 3 years ago

Hi @ZedThree, just wanted to check in to see if you had further comments.

ZedThree commented 3 years ago

Hi @landreman, sorry for the silence, I've been off on leave for a good chunk of August.

Overall, SIMSOPT is a really great package, and I would definitely recommend it to colleagues. It was easy to get started with it, lots of easy-to-follow examples, and a decent API documentation.

I had a similar comment to @StanczakDominik about SPEC, so I won't repeat that.

My other comment was that there didn't seem to be much in the way of documentation on the C++ part. I can see how most of it relates to the equivalent Python parts, but it would be good to have a few words in the documentation about it. I guess this is more important for developers than users.

dfm commented 3 years ago

@ZedThree, @StanczakDominik — Thanks a lot for your thorough reviews, I really appreciate the time you volunteered!

@landreman — Give me a couple of days to do some final editing and then I'll have a couple of last tasks for you.

landreman commented 3 years ago

@ZedThree Thanks for taking the time to look at the code and for your comments. I agree that the C++ components could use more comments and documentation - this is on my to-do list.

dfm 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:

dfm 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

- None

MISSING DOIs

- 10.2172/5537804 may be a valid DOI for title: Steepest-descent moment method for three-dimensional magnetohydrodynamic equilibria
- 10.2172/1072361 may be a valid DOI for title: Computation of multi-region relaxed magnetohydrodynamic equilibria
- 10.1088/1361-6587/abc08e may be a valid DOI for title: Coordinate parameterisation and spectral method optimisation for Beltrami field solver in stellarator geometry
- 10.13182/fst95-a11947086 may be a valid DOI for title: The Helically Symmetric Experiment, (HSX) Goals, Design and Status
- 10.1016/0010-4655(86)90109-8 may be a valid DOI for title: Representations for vacuum potentials in stellarators
- 10.2172/6191337 may be a valid DOI for title: A method for determining a stochastic transition
- 10.1201/9781003069515-49 may be a valid DOI for title: Elimination of stochasticity in stellarators
- 10.1016/0375-9601(88)90080-1 may be a valid DOI for title: Quasi-helically symmetric toroidal stellarators
- 10.1088/1741-4326/aaed50 may be a valid DOI for title: Optimisation of stellarator equilibria with ROSE
- 10.1103/physrevlett.80.528 may be a valid DOI for title: Transport Optimization and MHD Stability of a Small Aspect Ratio Toroidal Hybrid Stellarator
- 10.1088/1361-648x/ab82d2 may be a valid DOI for title: f90wrap: an automated tool for constructing deep Python interfaces to modern Fortran codes
- 10.1063/5.0061665 may be a valid DOI for title: Stellarator optimization for good magnetic surfaces at the same time as quasisymmetry

INVALID DOIs

- None
landreman commented 3 years ago

Oops, I hadn't realized I needed to include DOIs in the bibtex. I'll work on that now.

dfm commented 3 years ago

@landreman: I'll have a PR for you in moments, no worries.

dfm commented 3 years ago

@landreman — I've opened a pull request with some edits to the paper. Can you take a look at that and once you've merged, please take the following steps:

  1. Comment @whedon generate pdf from branch joss_paper on this thread and read through the manuscript to make sure that you're happy with it (it's hard to make changes later!), especially your name and affiliation.
  2. Increment the version number of the software and report that version number back here.
  3. Create an archived release of that version of the software (using Zenodo or something similar). Please make sure that the metadata (title and author list) exactly match the paper. Then report the DOI of the release back to this thread.

Let me know if you have questions or run into any issues!

landreman 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.1063/1.864116 is OK
- 10.1063/1.4765691 is OK
- 10.1088/1361-6587/abc08e is OK
- 10.1088/0741-3335/59/1/014018 is OK
- 10.13182/fst95-a11947086 is OK
- 10.1016/0010-4655(86)90109-8 is OK
- 10.1063/1.524170 is OK
- 10.1063/1.864692 is OK
- 10.1088/1741-4326/aa8e0a is OK
- 10.1016/0375-9601(88)90080-1 is OK
- 10.1038/s41592-019-0686-2 is OK
- 10.1088/1741-4326/aaed50 is OK
- 10.1103/physrevlett.80.528 is OK
- 10.1063/1.872844 is OK
- 10.1088/1361-648x/ab82d2 is OK
- 10.1063/5.0061665 is OK

MISSING DOIs

- None

INVALID DOIs

- None
landreman 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:

mbkumar commented 3 years ago

@dfm version number incremented to v0.4.7 DOI is 10.5281/zenodo.5498111 https://dx.doi.org/10.5281/zenodo.5498111

dfm commented 3 years ago

@whedon set v0.4.7 as version

whedon commented 3 years ago

OK. v0.4.7 is the version.

dfm commented 3 years ago

@whedon set 10.5281/zenodo.5498111 as archive

whedon commented 3 years ago

OK. 10.5281/zenodo.5498111 is the archive.

dfm commented 3 years ago

@whedon recommend-accept

whedon commented 3 years ago 
Attempting dry run of processing paper acceptance...
whedon commented 3 years ago

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

 Can't find any papers to compile :-(