search

openjournals / joss-reviews

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

[REVIEW]: dymos: A Python package for optimal control of multidisciplinary systems #2809

Closed whedon closed 3 years ago

whedon commented 3 years ago

Submitting author: @robfalck (Rob Falck) Repository: https://github.com/OpenMDAO/dymos Version: 1.0.0 Editor: @dpsanders Reviewer: @goerz, @thowell Archive: 10.5281/zenodo.4646412

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

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

@goerz & @thowell, 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 @dpsanders 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 @goerz

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @thowell

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. @goerz, @thowell 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 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1007/s00158-019-02211-z is OK
- 10.2514/3.21662 is OK
- 10.2514/6.2009-5989 is OK
- 10.2514/3.21224 is OK
- 10.2514/6.2020-3176 is OK
- 10.2514/6.2020-3141 is OK
- 10.2514/6.2017-4002 is OK
- 10.2514/6.2018-3738 is OK
- 10.2514/6.2019-4491 is OK
- 10.2514/6.2018-3884 is OK
- 10.21105/joss.02564 is OK
- 10.1145/2558904 is OK
- 10.2514/6.2006-6309 is OK
- 10.1145/838250.838251 is OK
- 10.2514/1.J052184 is OK
- 10.1002/adc2.18 is OK

MISSING DOIs

- 10.1007/s10107-004-0559-y may be a valid DOI for title: On the Implementation of an Interior-Point Filter Line-Search Algorithm for Large-Scale Nonlinear Programming
- 10.1007/s12532-018-0139-4 may be a valid DOI for title: CasADi – A software framework for nonlinear optimization and optimal control
- 10.1017/cbo9780511550157.005 may be a valid DOI for title: A mathematical view of automatic differentiation
- 10.1023/b:opte.0000048536.47956.62 may be a valid DOI for title: A coupled-adjoint sensitivity analysis method for high-fidelity aero-structural design
- 10.1016/j.paerosci.2019.05.002 may be a valid DOI for title: Effective adjoint approaches for computational fluid dynamics

INVALID DOIs

- https://doi.org/10.1038/s41592-019-0686-2 is INVALID because of 'https://doi.org/' prefix
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: @thowell, please update us on how your review is going.

whedon commented 3 years ago

:wave: @goerz, please update us on how your review is going.

goerz commented 3 years ago

I noticed that two people, @hwangjt and @kmarsteller seem to have contributed significantly to the software repo, but do not appear as authors on the paper. I don't have a fundamental problem with this, but @robfalck may want to comment on what these two people contributed to the project and why their contribution is not relevant for authorship on the paper, just so that I can mark that point in the review checklist with confidence.

@dpsanders: What exactly are JOSS' guidelines on this, beyond the "Does the full list of paper authors seem appropriate and complete?"

goerz commented 3 years ago

@robfalck The documentation of dymos points to https://arc.aiaa.org/doi/abs/10.2514/6.2019-0976, and that paper clearly already introduces the dymos package. That makes me somewhat inclined to reject the submission to JOSS as "not original". Can you comment on why you think the submission to JOSS is justified?

@dpsanders What are your guidelines on this? I would potentially consider a new major version of a software (or a "1.0" in this case) as warranting a second publication.

robfalck commented 3 years ago

I noticed that two people, @hwangjt and @kmarsteller seem to have contributed significantly to the software repo, but do not appear as authors on the paper. I don't have a fundamental problem with this, but @robfalck may want to comment on what these two people contributed to the project and why their contribution is not relevant for authorship on the paper, just so that I can mark that point in the review checklist with confidence.

Originally @hwangt helped with the transition from the original "alpha" stage of dymos (we called it pointer back then) into it's current form. At that point there were a few of us in the room coding together and he happened to be the one pushing the commits. In addition, he's not been on the project for several years and, and we opted to not list him as a co author.

@kmarstellar is recently retired. He's role in dymos was in aiding within interfacing to travis and helping getting the documentation working, but not with developing the core code. We've since moved on from using some of the documentation systems he implemented, and we opted to not include him as a coauthor.

robfalck commented 3 years ago

@robfalck The documentation of dymos points to https://arc.aiaa.org/doi/abs/10.2514/6.2019-0976, and that paper clearly already introduces the dymos package. That makes me somewhat inclined to reject the submission to JOSS as "not original". Can you comment on why you think the submission to JOSS is justified?

@dpsanders What are your guidelines on this? I would potentially consider a new major version of a software (or a "1.0" in this case) as warranting a second publication.

We viewed the AIAA paper as a theoretical discussion of "How should one approach the problem of trajectory optimization in an MDO context?" This paper documents the APIs and implementation of the software and how to use it. None of that was included in the AIAA publication. Furthermore, there have been a lot of capabilities that were developed since that paper was published:

goerz commented 3 years ago

Hmm.. ok with me, although it still seems a bit borderline, considering that the earlier paper announces Dymos in its abstract and extensively discusses benchmarks. In any case, this is clearly an editor's decision, so if @dpsanders has no objections to this manuscript being published in JOSS, I'll finish the review.

goerz commented 3 years ago

The JOSS manuscript will definitely have to cite the earlier publication, though. Even if it's just "The methods implemented in Dymos have previously been described in Ref"

dpsanders commented 3 years ago

@goerz: Many thanks for your detailed comments and review; apologies for the delay in replying. I'll give some responses below

dpsanders commented 3 years ago

@whedon generate pdf

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:

dpsanders commented 3 years ago

👋 @goerz and @robfalck. Having looked carefully at the prior paper https://arc.aiaa.org/doi/abs/10.2514/6.2019-0976, I agree that the previous paper was about the algorithms and a separate software publication in JOSS is acceptable.

I do agree with @goerz that it is an omission that needs to be rectified that that paper is not mentioned in the JOSS submission. @robfalck: Please be explicit about how the papers are different.

Given that you think of this paper as "document[ing] the APIs and implementation of the software and how to use it", perhaps you can go into a bit more detail about that, and how it differs from other packages. Thanks!

dpsanders commented 3 years ago

Regarding authorship, @robfalck's response seems fine to me.

dpsanders commented 3 years ago

👋 @thowell: I see that you have checked most of the boxes in the checklist at the top of this issue. If you have recommendations for the non-checked boxes, please feel free to raise them here or in issues in the software repo.

goerz commented 3 years ago

@dpsanders Thanks for weighing in!

I'm pretty much done with verifying the software package, and it's looking pretty good to me. The manuscript itself is ok, but I feel that at the moment it's still targeted at a very narrow audience, and I'm going to recommend some fixes to make it accessible to non-specialists. I'll write up my recommendations on the manuscript sometime later this week. After this small revision, I fully expect to be able to check off all items on the checklist and endorse the paper for publication.

@thowell I may have accidentally selected or unselected a couple of the checkmarks in your section :-/

thowell commented 3 years ago

@dpsanders I went through most of the boxes, but will add notes here as I finally get some free time early next week

thowell commented 3 years ago

I've basically finished my review. Two items:

dpsanders commented 3 years ago

👋 @goerz and @thowell: Just checking in to see if you are able to complete your reviews. Thanks!

goerz commented 3 years ago

I have checked the software package and its online documentation, and they appear ready for publication. The following are my comments on the manuscript itself. Again, sorry for the delay in writing this up...

The manuscript seems like it should be OK for someone within the narrow field of classical optimal control, e.g., users of some of the existing packages cited in the manuscript. However, I think the Summary could be improved to expand the scope towards a more "diverse, non-specialist audience". I myself seem like a good "test" audience, in fact: My background is in quantum optimal control theory, where I routinely optimize processes to drive a complex-valued initial state-vector toward some desired target vector, subject to constraints. There is a well-established set of methods and software tools to solve these control problems, without much overlap to the classical control community. However, at least in principle, it would seem that there could be more overlap between the communities, as I could rewrite my complex state vectors to real-valued vectors by separating real and imaginary parts. At least to me personally it is an open question whether and to what extent there are possible improvements or insights to be gained from classical control theory software. I would read a paper like the Dymos JOSS manuscript with this question in my; How would I have to reformulate my quantum control problems so that it would fit into Dymos' (or any similar package's) language.

First, I would recommend that the manuscript uses some (minimal) mathematical notation to be more specific without requiring the reader to be familiar with domain-specific jargon. The mathematical definitions in https://openmdao.github.io/dymos/getting_started/optimal_control.html#the-overall-optimization-problem are something I can understand, and I would recommend putting them in the manuscript, at least in some shortened form. This would then also allow to specify Dymos' unique "co-design" feature:

Dymos [...] is unique in that it can also support broader cases where trajectory optimization is just one component in a larger physical model.

I don't understand what that means: How does the "larger physical model" not fit into the equations describing the control objectives and system dynamics? Are these not numerically accessible? Can you give a specific easy to understand example (like from the cited aircraft design paper): What is the "pure" optimal control problem here, and what is the larger physical model?

Is the presence of the DAE's (second paragraph) what you mean by the "larger physical model"? Again, a specific example or mathematical definition might help (what's an algebraic equation again?) Or, is the "co-design" just the presence of different phases (second section), with enforced boundary conditions between the phases?

One application of this approach is the method of differential inclusions

Dymos does "differential inclusion" right? (just using a "modular" approach, instead of "monolithic algorithmic differentiation") It's not 100% clear if the "Instead" that starts the third paragraph refers to the entire second paragraph ("differential inclusion") or just the "monolithic" part. If my reading is correct, then the paragraph break should before the sentence "Some optimal control libraries tackle ..."

You might want to rewrite this part slightly, in any case:

While effective, the monolithic nature is both less efficient(Kenway et al., 2019; Mader et al., 2008) and less flexible.

Less than what? Less than Dymos' approach? I would probably turn this around: After introducing "differential inclusions" in paragraph two, introduce what Dymos does in paragraph three, and end with something like "The modular approach is both more efficient and more flexible than tackling the numerical challenge with a monolithic algorithmic differentiation ...". Has the "modular approach" been introduced before (citation?). What are the citations from Kenway and Mader comparing?

The available time discretization schemes in Dymos are based on pseudospectral methods, using two common direct collocation transcriptions: the high-order Gauss-Lobatto transcription (Herman & Conway, 1996) and the Radau pseudospectral method (Garg et al., 2009). Both implicit and explicit forms of these transcriptions are supported. The explicit forms can be used to build single or multiple-shooting style problem formulations. The implicit forms can be used to construct two point boundary value problems. Transcriptions are built to be totally independent of the ODE implementation, and nearly transparent to the user. This means that switching transcriptions requires very minor code changes - typically a single line in the run-script.

This paragraph has a lot of domain-specific jargon: collocation, transcriptions, two-point boundary problem, shooting style problems (is that what I would call "gradient-free"?). This is fine for a reader who is familiar with all of these, but it might be worth the effort to add a couple of sentences with high-level explanations.

As for the example, I have to agree with @thowell. It's a relatively straightforward optimization problem, and illustrates the basic usage of Dymos nicely, but it doesn't really show Dymos' unique features. Specifically, it doesn't give me an example for the fundamental "co-design" feature (my initial question)

Small Typos

in order to solve ODE

in order to solve the ODE(s)

fearures

features (please do run a spell-checker!)

Summary

I think the manuscript could use substantial work if the authors want to reach a wide audience (which JOSS would seem to encourage), while for a narrow audience, it seems mostly fine in its current form. I will leave it up to the authors to decide to what extent they want to revise based on my comments above. After revision, I would endorse the paper for publication.

I have left the related boxes in the checklist open for now, but will be happy to check them on revision.

Happy Holidays!

robfalck commented 3 years ago

@goerz

Thank you for your feedback. My coauthors and I agree with your point about broadening the audience and have added more detail there. We added a discussion and diagrams covering more use-cases for more multidisciplinary/co-design applications. While you're right that optimal control often involves modeling the static system model as part of the ODE, in our experience there are applications where it's better to separate the static design and optimal control portions of the optimization. We added our simple cannonball design example to the paper to demonstrate the separation of the static and dynamic design elements. It's somewhat difficult to capture a real-world case in a short format such as this paper, since more realistic applications generally involve many more models and hundreds or thousands of lines of code.

We've also included the mathematical notation describing the optimal control problems that Dymos can solve.

Regarding approaching quantum applications, that is interesting and we do have other OpenMDAO-based tools for circuit design that implement complex variables as separated real variables.

We hope our other clarifications address some of your points.

@thowell

Hopefully the simple cannonball design example, while contrived, is a bit more demonstrative of the sort of multidisciplinary/co-design capability we're focusing on.

Regarding performance, there is nothing in this paper but we do provide some comparison against OTIS in our paper on the methods behind Dymos that we previously discussed (http://openmdao.org/pubs/falck_dymos_2019_scitech.pdf). OTIS is a Boeing/NASA-developed legacy optimal control tool. OTIS is written in Fortran and takes advantage of sparsity, but uses finite differencing to obtain derivatives for the optimizer. We compared against the supersonic minimum time-to-climb problem. While OTIS is faster for small numbers of "nodes" in the trajectory (the number of points at which the continuous solution is discretized), Dymos scales better as the number of nodes is increased. It's not surprising that Fortran would be faster than Python on small problems, but the more efficient derivative calculation methods used in Dymos overcome that as the problem size grows larger.

Thank you both for your reviews.

dpsanders commented 3 years ago

:wave @robfalck: Just checking in to see what the status of your manuscript is. Have the comments of the reviewers been incorporated? Thanks!

robfalck commented 3 years ago

@dpsanders Yes I think my commits from 15 days ago take into account their comments.

dpsanders commented 3 years ago

@whedon generate pdf

whedon commented 3 years ago

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

Error producing PDF. ! LaTeX Error: File `setspace.sty' not found.

Type X to quit or to proceed, or enter new name. (Default extension: sty)

Enter file name: ! Emergency stop. <read *>

l.328 ^^M

Looks like we failed to compile the PDF

dpsanders commented 3 years ago

@robfalck: There's a problem with the PDF compilation^

dpsanders commented 3 years ago

(Note that you can check this using https://whedon.theoj.org/)

robfalck commented 3 years ago

Hmm I'll check that, thanks.

robfalck commented 3 years ago

It's very odd, we've been using the github action to compile the paper and it's been fine (including rebuilding it a few times today). Are there ever issues with whedon?

dpsanders commented 3 years ago

There are definitely sometimes issues with Whedon, yes. Maybe we can try again in a while, and otherwise I'll draw somebody's attention to it. Thanks!

dpsanders commented 3 years ago

@whedon generate pdf

whedon commented 3 years ago

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

Error producing PDF. ! LaTeX Error: File `setspace.sty' not found.

Type X to quit or to proceed, or enter new name. (Default extension: sty)

Enter file name: ! Emergency stop. <read *>

l.328 ^^M

Looks like we failed to compile the PDF

robfalck commented 3 years ago

I have a fix for this, should be up shortly.

robfalck commented 3 years ago

@whedon generate pdf

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:

robfalck commented 3 years ago

@dpsanders fixed

goerz commented 3 years ago

I'll read through it by the end of the week. At first glance, it looks very much improved. I immediately saw some typos, though (like, 3rd paragraph, 1st sentence: missing "as"), so you might want to proof-read again.

robfalck commented 3 years ago

@whedon generate pdf

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:

goerz commented 3 years ago

I've re-read the manuscript in its current, updated version. I'm happy to see that the authors substantially revised the text, resulting in a much more accessible publication. I have no fundamental objections to seeing the manuscript published in its current form.

However, the writing is sloppy, and further editing will be required before publication. A thorough proof-reading should have caught most of these issues. I expect authors to proof-read their submissions before sending them out for review!

A few of the issues:

I probably missed some additional typos and/or grammatical errors. I'm not going to read through this again, so please make sure to fix them.

robfalck commented 3 years ago

These have been addressed (in addition to a few others).

robfalck commented 3 years ago

@whedon generate pdf

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:

goerz commented 3 years ago

Missing comma at end of line 142; "tend" in line 144 should be "tends"

robfalck commented 3 years ago

@whedon generate pdf

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:

robfalck commented 3 years ago

@dpsanders I think I've addressed all of the concerns here. Please let me know what else I need to do.

kthyng commented 3 years ago

Looks like this paper is just about complete! Because of this I thought I'd just check in: @goerz, @thowell can you verify if your reviews are complete?