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

[REVIEW]: DelayDiffEq: Generating Delay Differential Equation Solvers via Recursive Embedding of Ordinary Differential Equation Solvers #125

Open
36 of 42 tasks
whedon opened this issue Mar 7, 2023 · 22 comments
Assignees

Comments

@whedon
Copy link
Collaborator

whedon commented Mar 7, 2023

Submitting author: @devmotion (David Widmann)
Repository: https://github.com/SciML/DelayDiffEq.jl
Branch with paper.md (empty if default branch):
Version:
Editor: @odow
Reviewers: @Datseris, @ranocha
Archive: Pending

Status

status

Status badge code:

HTML: <a href="https://proceedings.juliacon.org/papers/c8cb32ec0ee3f20ca54c35f22eaf2004"><img src="https://proceedings.juliacon.org/papers/c8cb32ec0ee3f20ca54c35f22eaf2004/status.svg"></a>
Markdown: [![status](https://proceedings.juliacon.org/papers/c8cb32ec0ee3f20ca54c35f22eaf2004/status.svg)](https://proceedings.juliacon.org/papers/c8cb32ec0ee3f20ca54c35f22eaf2004)

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

@Datseris & @ranocha, 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 @odow 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 @Datseris

Conflict of interest

Code of Conduct

General checks

  • Repository: Is the source code for this software available at the repository url?
  • License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
  • Authorship: Has the submitting author (@devmotion) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?

Functionality

  • Installation: Does installation proceed as outlined in the documentation?
  • Functionality: Have the functional claims of the software been confirmed?
  • Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

  • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
  • Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
  • Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
  • Automated tests: Are there automated tests or manual steps described so that the function of the software can be verified?
  • Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Paper format

  • Authors: Does the paper.tex file include a list of authors with their affiliations?
  • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • References: Do all archival references that should have a DOI list one (e.g., papers, datasets, software)?
  • Page limit: Is the page limit for full papers respected by the submitted document?

Content

  • Context: is the scientific context motivating the work correctly presented?
  • Methodology: is the approach taken in the work justified, presented with enough details and reference to reproduce it?
  • Results: are the results presented and compared to approaches with similar goals?

Review checklist for @ranocha

Conflict of interest

Code of Conduct

General checks

  • Repository: Is the source code for this software available at the repository url?
  • License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
  • Authorship: Has the submitting author (@devmotion) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?

Functionality

  • Installation: Does installation proceed as outlined in the documentation?
  • Functionality: Have the functional claims of the software been confirmed?
  • Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

  • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
  • Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
  • Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
  • Automated tests: Are there automated tests or manual steps described so that the function of the software can be verified?
  • Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Paper format

  • Authors: Does the paper.tex file include a list of authors with their affiliations?
  • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • References: Do all archival references that should have a DOI list one (e.g., papers, datasets, software)?
  • Page limit: Is the page limit for full papers respected by the submitted document?

Content

  • Context: is the scientific context motivating the work correctly presented?
  • Methodology: is the approach taken in the work justified, presented with enough details and reference to reproduce it?
  • Results: are the results presented and compared to approaches with similar goals?
@whedon
Copy link
Collaborator Author

whedon commented Mar 7, 2023

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @Datseris, @ranocha it looks like you're currently assigned to review this paper 🎉.

⚠️ JOSS reduced service mode ⚠️

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.

⭐ Important ⭐

If you haven't already, you should seriously consider unsubscribing from GitHub notifications for this (https://github.com/JuliaCon/proceedings-review) 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/JuliaCon/proceedings-review:

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

whedon commented Mar 7, 2023

Software report (experimental):

github.com/AlDanial/cloc v 1.88  T=0.05 s (1253.8 files/s, 109376.6 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Julia                           48            805            506           3568
YAML                             6             13             10            172
Markdown                         2             14              0             78
TOML                             3              3              0             45
TeX                              1              1              0             19
-------------------------------------------------------------------------------
SUM:                            60            836            516           3882
-------------------------------------------------------------------------------


Statistical information for the repository 'f805eeb45b8dccf7eae368f6' was
gathered on 2023/03/07.
No commited files with the specified extensions were found.

@whedon
Copy link
Collaborator Author

whedon commented Mar 7, 2023

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

 Can't find any papers to compile :-(

@devmotion
Copy link

@whedon generate pdf from branch paper

@whedon
Copy link
Collaborator Author

whedon commented Mar 7, 2023

Attempting PDF compilation from custom branch paper. Reticulating splines etc...

@whedon
Copy link
Collaborator Author

whedon commented Mar 7, 2023

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

@ranocha
Copy link
Collaborator

ranocha commented Mar 8, 2023

The links to the conflict of interest and code of conduct policies do not work

@odow
Copy link
Member

odow commented Mar 8, 2023

@Datseris
Copy link
Collaborator

Alright, I am submitting my review!

Summary

This is an excellently written paper. It clearly introduces the type of modelling it is about, why modelling this is useful for the scientific community, and why modelling this is difficult from both a technical and a conceptual viewpoint.

The paper the proceeds in explaining clearly the brilliant design of the library and its integration with OrdinaryDiffEq.jl. This is very important. To my knowledge, OrdinaryDiffEq.jl is by far the most featureful and most performant software for solving ordinary differential equations. Integrating with that brings so many benefits into the table practically "for free". The authors have employed several "tricks" to make this integration possible, such as linking caches between the ODE solver, and the DDE solver, which in fact includes the ODE solver, or employing special handling for discontinuities, which seems to be one of the hardest technical problems in DDE solving.

All in all, this is an incredible piece of work and I would highly recommend publication!

Code

I was able to install the software with one command. I was able to run its test suite with passing tests. The test suite was very extensive, yet one more thing I found impressive. The example tutorial was clear and straightforward, and I've also run it successfully.

Comments

  • "A disadvantage of the method of steps that it can be inefficient." needs "is" before "that".
  • There are no "installation instructions" in the sense of I couldn't find anywhere the statement "To install run Pkg.add("DelayDiffEq")".
  • It is difficult from the documentation to tell which features are part of DelayDiffEq.jl and which features are part of the greater ecosystem. Reason for this difficulty is that DelayDiffEq.jl is not independently documented, but rather documented directly in the whole DifferentialEquations.jl-wide documentation. My review checklist requires me to check that the Functionality of the software is documented, and this is the only tick I haven't checked off yet. I think I need the authors to clarify this for me. Does the page https://docs.sciml.ai/DiffEqDocs/stable/tutorials/dde_example/ contain all functionality of the software?

@whedon
Copy link
Collaborator Author

whedon commented Mar 21, 2023

👋 @Datseris, please update us on how your review is going (this is an automated reminder).

@whedon
Copy link
Collaborator Author

whedon commented Mar 21, 2023

👋 @ranocha, please update us on how your review is going (this is an automated reminder).

@ranocha
Copy link
Collaborator

ranocha commented Apr 5, 2023

@whedon generate pdf from branch paper

@whedon
Copy link
Collaborator Author

whedon commented Apr 5, 2023

Attempting PDF compilation from custom branch paper. Reticulating splines etc...

@whedon
Copy link
Collaborator Author

whedon commented Apr 5, 2023

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

@ranocha
Copy link
Collaborator

ranocha commented Apr 5, 2023

Thanks for all the work and this manuscript. It should be published after some revisions. In addition to the comments of @Datseris above, please consider the following comments.

  • Page 2, "is a scalar DDE system". Usually, people would call it a scalar DDE and not a system since it has only one component.
  • Page 2, "Additionally, problems with vanishing delays cannot be solved by the method of steps.". I think this can be confusing. You write about DDEs with a fixed delay. If the delay vanishes, it's just an ODE, isn't it?
  • Page 3: You talk about convergence failures. What exactly does that mean - your fixed-point iteration fails to converge? What do you do in this case?
  • It would make sense to cite the articles developing the methods you mention in your manuscript.
  • Page 4: It would help people not familiar with OrdinaryDIffEq.jl if you explained the parameters briefly, e.g., that p can be some parameters (that will just be ignored in this example).
  • JuliaCon Proceedings Review: Example fails SciML/DelayDiffEq.jl#265
  • You have argued that the method of steps is inefficient and bad on page 2. However, you present quite a lot of examples where you use MethodOfSteps as algorithm. This is quite confusing. Is there an alternative?
  • Page 4, "the most costly aspect for solving large stiff equations". I would argue that you just cannot factorize a Jacobian for a large system where you need thousands of MPI processes.
  • Page 4, eq. (7): This is a state-dependent delay since you need something like $x_1(x_6(t))$, correct? It may be nice to point this out explicitly for readers curious about the technology but not necessarily familiar with all DDE examples.
  • Reference [15] is incomplete.
  • Some references such as [4] are lacking DOIs.
  • It would be nice to clarify which of the arguments listed in https://docs.sciml.ai/DiffEqDocs/stable/types/dde_types/ are keyword arguments, e.g., by introducing an additional header "Keyword arguments" after the description of the "Arguments".
  • There is no CONTRIBUTING.md or something similar
  • It would be nice to add the same standard badges to the README of DelayDiffEq.jl as for other repos such as OrdinaryDiffEq.jl (and maybe also a link to the documentation next to the GitHub summary). Right now, there is no obvious way to get to the docs when viewing the upper part of the GitHub website of DelayDiffEq.jl.

image

@ranocha
Copy link
Collaborator

ranocha commented Apr 5, 2023

@whedon generate pdf from branch paper

@whedon
Copy link
Collaborator Author

whedon commented Apr 5, 2023

Attempting PDF compilation from custom branch paper. Reticulating splines etc...

@whedon
Copy link
Collaborator Author

whedon commented Apr 5, 2023

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

@devmotion
Copy link

@ranocha @Datseris Thank you for your reviews, the positive feedback, and the valuable suggestions!

I'm currently in the final stages of writing my thesis, so it will probably take a few days until I am able to address all your comments. I'll post in this thread and reply to your comments in more details when I am done with some initial revisions.

@odow
Copy link
Member

odow commented Sep 11, 2023

@devmotion what's the status of this?

@odow
Copy link
Member

odow commented Nov 10, 2023

Hi @devmotion just checking in on this.

@odow
Copy link
Member

odow commented May 16, 2024

Hi @devmotion just checking in on this again. Let me know if you need anything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants