search

openjournals / joss-reviews

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

[REVIEW]: pyOptSparse: A Python framework for large-scale constrained nonlinear optimization of sparse systems #2564

Closed whedon closed 4 years ago

whedon commented 4 years ago

Submitting author: !--author-handle-->@ewu63<!--end-author-handle-- (Ella Wu) Repository: https://github.com/mdolab/pyoptsparse/ Version: v2.2.1 Editor: !--editor-->@poulson<!--end-editor-- Reviewer: @jgoldfar, @vissarion, @matbesancon Archive: 10.5281/zenodo.4110792

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

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

@jgoldfar & @vissarion & @matbesancon, 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 @poulson 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 @jgoldfar

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @vissarion

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @matbesancon

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 4 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @jgoldfar, @vissarion, @matbesancon 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 4 years ago

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

Can't find any papers to compile :-(

poulson commented 4 years ago

:warning: As noted by @vissarion and @matbesancon in the pre-review, both of them expect to be unable to start their review until September. :warning:

poulson commented 4 years ago

@whedon generate pdf from branch paper

whedon commented 4 years ago 
Attempting PDF compilation from custom branch paper. Reticulating splines etc...
whedon commented 4 years ago

:point_right: Check article proof :page_facing_up: :point_left:

matbesancon commented 4 years ago

@whedon remind @matbesancon in 3 weeks

whedon commented 4 years ago

I'm sorry @matbesancon, I'm afraid I can't do that. That's something only editors are allowed to do.

matbesancon commented 4 years ago
  1. Comparisons

I took a preliminary look at the paper, it would be interesting to compare the features with other optimization packages including solvers like Ipopt, SCIP and their Python bindings and solver-agnostic modelling languages like Pyomo or JuMP. Is PyOpt more a solving or modeling package. If it is of the latter, what solvers are currently supported and how easy is it to switch between solvers?

  1. Usage

Their could be a section focused more on usage of the package. Non-linear optimization is notoriously hard to make user-friendly. There could be a representative usage example. How are non-linear functions passed to the packages? Are arbitrary Python functions supported?

ewu63 commented 4 years ago

Hi Mathieu, thanks for taking your time to do this review.

  1. pyOptSparse is a wrapper for several popular optimizers, with a particular focus on large-scale nonlinear programming of continuous variables. As such, it is closer to frameworks such as pyomo and scipy, where the optimization problem is defined in a particular syntax and pyOptSparse performs the conversion to the specific form required for each optimizer. The main distinguishing feature for pyOptSparse is the support for sparse constraint Jacobians, both for linear and nonlinear constraints, and particularly for supporting sparse sub-blocks of the Jacobian. Through the unified framework, it's straightforward to switch between say SNOPT and IPOPT by changing one line of code, and all conversion will be done automatically including converting the sparse Jacobian into a different format. There are also other features such as the ability to perform finite-difference or complex-step to compute derivatives automatically, and MPI support for parallel function evaluations.

    The list of solvers is shown in the documentation (for example the side bar here). To switch between optimizers, the user would need to 1) change the options dictionary, and 2) change the name of the optimizer in the optimize call, for example replacing SLSQP with IPOPT in the line opt = OPT("SLSQP", options=optOptions). I hesitate to provide a list and description of the optimizers in the paper because the list may change over time, with older optimizers eventually become deprecated and new ones added.

  2. Yes, arbitrary Python functions are supported, and the data is stored in a dictionary. A nested dictionary is used for gradients if they are supplied. It's a little bit unclear for me what type of information should be in the paper, and what should be in the documentation site of the repository. I would prefer, if possible, to keep most of this information in the documentation since API may change and users typically will refer to that instead of the journal paper for help on using the package. But please let me know if JOSS takes a different view and I'd be happy to add more information to the paper. For reference, please see the Quickstart for a basic example, and the Guide for a complete example with sparse linear and nonlinear constraints.

matbesancon commented 4 years ago

I agree that listing the solvers is not super relevant to the paper which should be more of a permanent document presenting the package. The fact that multiple solvers can be used with the same uniform syntax should be highlighted more.

There are also other features such as the ability to perform finite-difference or complex-step to compute derivatives automatically, and MPI support for parallel function evaluations.

I think this could be presented in the paper, as it can be of interest to the reader comparing different frameworks.

For the second point, I think more should go in the paper without going into the internal details that are likely to change. I think having at least a high-level example with arbitrary Python functions would be of value to the article

ewu63 commented 4 years ago

Hi Mathieu, I agree with your points and I will push an update to the paper soon.

vissarion commented 4 years ago

I finished my review.

I agree with the points listed above. I would also like to see a high-level example probably with the comment that links to the documentation for more details.

One more comment, I think it would be useful to note in the paper the current release version of the package.

ewu63 commented 4 years ago

Hi Mathieu and Vissarion,

Thanks for your feedback and apologies for not replying sooner, but I have been working with co-authors to update the paper. There are some substantial changes:

The paper is located under the paper branch of the repo, please take a look and let me know if you have any feedback.

Cheers, Neil

vissarion commented 4 years ago

Thanks for the substantial revision Neil. The paper now looks much better and useful for the reader. I do not have further comments.

matbesancon commented 4 years ago

@whedon generate pdf from branch paper

whedon commented 4 years ago 
Attempting PDF compilation from custom branch paper. Reticulating splines etc...
whedon commented 4 years ago

:point_right: Check article proof :page_facing_up: :point_left:

matbesancon commented 4 years ago

Posted on https://github.com/mdolab/pyoptsparse/issues/145 for the testing part which seems to have issues

matbesancon commented 4 years ago

Ok nevermind the tests do work. On the installation instructions here, I would recommend you state upfront that a fortran compiler is needed (maybe recommend gfortran directly)

ewu63 commented 4 years ago

I've updated the installation instructions in this PR.

matbesancon commented 4 years ago

perfect thanks!

matbesancon commented 4 years ago

@jgoldfar I might have checked your tick boxes by accident, not sure

ewu63 commented 4 years ago

This is a friendly reminder now that we're coming up on 6 weeks, @matbesancon and @jgoldfar are you done with the review? Also @poulson I noticed that Jonathan is not assigned to this issue, I don't know if that causes anything. Thanks!

ewu63 commented 4 years ago

Hi @poulson and @labarba, is there anything for me to do to get things moving again?

matbesancon commented 4 years ago

hi, sorry about the delay on coming back to this review, I'll be on it in the coming days

matbesancon commented 4 years ago

Regarding the paper, thanks to the authors for the modifications. It is much clearer as-is. Few additional comments and remarks:

These are the last points I consider worth expanding before the paper is ready

ewu63 commented 4 years ago

Hi Mathieu, thanks for the detailed review, I'll address the code issues shortly. As for your questions:

Please let me know if this was not clear from the paper, and how I can improve it further.

Thanks again, Neil

matbesancon commented 4 years ago

Thanks for your answer. For the motivation, I feel the first point (telling the optimizer which entries of the Jacobian will always be zero), this is a performance reason, hence my question of comparing it to a dense formulation or modelling library. In any case, the motivation could be expanded a bit in the statement of need section.

The use case for the history (and the way it works) could also be more detailed in the paper, otherwise the reader might not understand what this implies for them as users

ewu63 commented 4 years ago

I'll update the paper, as well as address the opened issues. Thanks again!

ewu63 commented 4 years ago

@matbesancon the issues you opened have been addressed, and I also pushed a minor update to the paper.

@poulson does JOSS require three reviewers? As far as I can tell @jgoldfar has not started his review yet.

ewu63 commented 4 years ago

@whedon generate pdf from branch paper

whedon commented 4 years ago 
Attempting PDF compilation from custom branch paper. Reticulating splines etc...
whedon commented 4 years ago

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

matbesancon commented 4 years ago

@poulson I finished my review. This is a great paper covering a very interesting software package. The article highlights the interest optimization practitionners can have using it compared to other modelling libraries. I recommend its acceptation for JOSS

poulson commented 4 years ago

Hi @nwu63 -- with respect to your three reviewer question, given that there are two very positive reviews I think that is sufficient.

I will procede by giving it a proofread and then walk through the checklist for the final approval.

Thank you all for your patience, as I fell into a bit of a spiral of knowing I was behind on checking in and avoiding circling back.

poulson commented 4 years ago

@whedon check references

poulson commented 4 years ago

@whedon commands

whedon commented 4 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 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
poulson commented 4 years ago

@whedon check references

poulson commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago

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

Can't find any papers to compile :-(

poulson commented 4 years ago

@whedon check references from branch paper

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

OK DOIs

- 10.1007/s00158-011-0666-3 is OK
- 10.2514/1.J056603 is OK
- 10.2514/1.C034934 is OK
- 10.5194/wes-4-163-2019 is OK
- 10.1007/s00158-019-02211-z is OK
- 10.1007/978-3-319-97773-7_38 is OK
- 10.1017/aer.2018.120 is OK
- 10.1145/838250.838251 is OK

MISSING DOIs

- None

INVALID DOIs

- https://doi.org/10.1038/s41592-019-0686-2 is INVALID because of 'https://doi.org/' prefix
poulson commented 4 years ago

@nwu63 -- I just reread the paper, and I have to say, this is one of my favorite JOSS submissions.

Would you mind fixing the trivial DOI issue mentioned above and then providing a DOI for your source code archive (e.g., from Zenodo)?

ewu63 commented 4 years ago

@poulson: Thanks! This has been a really good experience and I appreciate all the work you guys do.

I am planning to make a new code release so that the updated code is archived by zenodo. I just wanted to clarify if the code release should contain the paper? i.e. should I merge the content of the paper branch into master before that release. I saw from some other JOSS reviews that you prefer having the zenodo metadata match the paper, so I will add the json metadata file to remedy that as well.

ewu63 commented 4 years ago

@poulson I have just made a new release which also merged the contents of the paper. The zenodo archive is also there now with the correct author list, the DOI is 10.5281/zenodo.4110792.

ewu63 commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago

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