search

openjournals / joss-reviews

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

[REVIEW]: QuantizedSystemSolver: A discontinuous ODE system 2 solver for Julia. #7434

Open editorialbot opened 2 weeks ago

editorialbot commented 2 weeks ago

Submitting author: !--author-handle-->@mongibellili<!--end-author-handle-- (elmongi elbellili) Repository: https://github.com/mongibellili/QuantizedSystemSolver.jl Branch with paper.md (empty if default branch): Version: v1.0.1 Editor: !--editor-->@diehlpk<!--end-editor-- Reviewers: @joshuaeh, @lamBOOO Archive: Pending

Status

status

Status badge code:

HTML: <a href="https://joss.theoj.org/papers/f779f8dbd5bcd59e354eb9dddda30eac"><img src="https://joss.theoj.org/papers/f779f8dbd5bcd59e354eb9dddda30eac/status.svg"></a>
Markdown: [![status](https://joss.theoj.org/papers/f779f8dbd5bcd59e354eb9dddda30eac/status.svg)](https://joss.theoj.org/papers/f779f8dbd5bcd59e354eb9dddda30eac)

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

@joshuaeh & @lamBOOO, your review will be checklist based. Each of you will have a separate checklist that you should update when carrying out your review. First of all you need to run this command in a separate comment to create the checklist:

@editorialbot generate my checklist

The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @diehlpk 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 ✨

Checklists

📝 Checklist for @lamBOOO

📝 Checklist for @joshuaeh

editorialbot commented 2 weeks ago

Hello humans, 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 commented 2 weeks ago

Software report:

github.com/AlDanial/cloc v 1.90  T=0.35 s (486.0 files/s, 86155.1 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Julia                          121           3297           6837          16499
Markdown                        14            179              0            986
JavaScript                       4            137            182            879
HTML                            12             53              0            310
TOML                             3             59              1            238
YAML                             7              6              9            230
TeX                              1              1              0             36
CSS                              6              1             13              6
JSON                             1              0              0              1
-------------------------------------------------------------------------------
SUM:                           169           3733           7042          19185
-------------------------------------------------------------------------------

Commit count by author:

    57  Mongi Bellili
    35  mongibellili
    33  mongi bellili
     1  Daan Huybrechs
editorialbot commented 2 weeks ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

✅ OK DOIs

- 10.5281/zenodo.2557003 is OK
- 10.1137/141000671 is OK

🟡 SKIP DOIs

- No DOI given, and none found for title: MacroTools.jl
- No DOI given, and none found for title: SymEngine.jl
- No DOI given, and none found for title: Improving linearly implicit quantized state system...

❌ MISSING DOIs

- None

❌ INVALID DOIs

- None
editorialbot commented 2 weeks ago

Paper file info:

📄 Wordcount for paper.md is 656

✅ The paper includes a Statement of need section

editorialbot commented 2 weeks ago

License info:

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

editorialbot commented 2 weeks ago

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

lamBOOO commented 2 weeks ago

Review checklist for @lamBOOO

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

joshuaeh commented 2 weeks ago

Review checklist for @joshuaeh

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

lamBOOO commented 2 weeks ago

@editorialbot commands

editorialbot commented 2 weeks ago

Hello @lamBOOO, here are the things you can ask me to do:


# List all available commands
@editorialbot commands

# Get a list of all editors's GitHub handles
@editorialbot list editors

# Adds a checklist for the reviewer using this command
@editorialbot generate my checklist

# Set a value for branch
@editorialbot set joss-paper as branch

# Run checks and provide information on the repository and the paper file
@editorialbot check repository

# Check the references of the paper for missing DOIs
@editorialbot check references

# Generates the pdf paper
@editorialbot generate pdf

# Generates a LaTeX preprint file
@editorialbot generate preprint

# Get a link to the complete list of reviewers
@editorialbot list reviewers
lamBOOO commented 1 week ago

Review of QuantizedSystemSolver.jl

The Julia package QuantizedSystemSolver.jl implements the quantized state method for stiff ordinary differential equations (ODEs), where different solution variables are updated only when they surpass a threshold (quantization). This approach allows for efficient treatment of stiff ODEs that typically involve multiple time scales in their evolution, eliminating the need for a very fine global time step. The package appears to implement the core functionality and provides some API documentation along with three examples.

While the software is easily installable, the README could benefit from updates and improvements. Additionally, there should be more discussion, testing, and demonstration of why and how the quantized approach compares to classical methods. The current interface for problem setup using Julia's macro programming may not be the most optimal solution.

I have attached a list of issues in the original repository that I would like to see addressed, before checking the boxes in the checklist.

The package shows promise and could be a valuable addition to the field, but it requires significant revisions. I am willing to review it again once the issues have been addressed.

Substantial Scholarly Effort

Issues in Original Repository

Full Report

## Software ### General - **Remove Empty Files**: Files such as `.github/workflow` are empty and should be removed. - **Clean Up Redundant Directories**: The `build` folder appears to be a partial copy of `src`. If it is unnecessary, it should be removed. - **Improve Documentation**: Not all functions are documented with doc strings. Comprehensive documentation would enhance usability. - **Eliminate Unnecessary Comments**: The code contains many comments that may not be needed, for example, in `tests/results_systems.jl`. Removing them can improve code readability. - **Simplify Build Scripts**: The `docs/make.jl` file contains unnecessary code, such as `run(`cp -r "C:/Users/belli/.julia/dev/QuantizedSystemSolver/docs/build/*"`)`. This should be cleaned up. - **Remove Unnecessary Files**: - `lcov.info` - `src/dense/NL_integrators/workspace.code-workspace` - Please review the repository thoroughly to remove files that are not relevant to users. - **Standardize Directory Naming**: Some folders within `src` use uppercase letters while others use lowercase. Consistent naming conventions should be applied. ### Design - **Use of `quote ... end` in Problem Definition**: The `NLodeProblem` utilizes the `quote ... end` construct, resulting in an `Expr` object. For example: ```julia odeprob = NLodeProblem(quote name=(sysb53,) u = [-1.0, -2.0] du[1] = -20.0*u[1]-80.0*u[2]+1600.0 du[2] = 1.24*u[1]-0.01*u[2]+0.2 end) ``` This contrasts with a more conventional approach: ```julia ode_prob = NLodeProblem( name = sysb53, u0 = [-1.0, -2.0], # Initial condition for u f = function (du, u) du[1] = -20.0 * u[1] - 80.0 * u[2] + 1600.0 du[2] = 1.24 * u[1] - 0.01 * u[2] + 0.2 end ) ``` - **Concerns**: This unconventional use of `quote ... end` may not be the most elegant or efficient method in terms of type inference, robustness, and API accessibility. - **Recommendation**: Please explain the rationale behind this design choice or consider adopting the more standard method of using function arguments in Julia. The current approach makes it unclear what the function does with the input provided within the `quote ... end` block. ## README - **Include Logo**: The documentation features a logo; consider adding it to the README for consistency and branding purposes. - **Provide Usage Instructions**: The README should include instructions on how to execute the software's functionality, including the necessary `using ...` commands to import the package. - **Fix Code Errors**: The following code snippet from the README returns an error: ```julia # Returns a vector of the values of all variables at time 0.0005 (feature not available in v1.0.1) sol(0.0005) ``` **Error Message**: ``` ERROR: MethodError: no method matching (::QuantizedSystemSolver.LightSol{2, 2})(::Float64) Closest candidates are: (::Sol)(::Int64, ::Float64) @ QuantizedSystemSolver ~/.julia/packages/QuantizedSystemSolver/XwCgz/src/Common/Solution.jl:93 Stacktrace: [1] top-level scope @ REPL[17]:1 ``` This suggests that the feature may not be available or properly implemented. Please resolve this issue or update the README to reflect the current capabilities. - **Mathematical Explanation of the ODE Problem**: The ODE problem presented in the README should be accompanied by a mathematical explanation. This will help users understand how to formulate their own problems and apply the software effectively. - **Clarify Feature Availability**: The README mentions several features marked as "available only in v1.0.1." This can be confusing for users. Please update the README to reflect the current version and available features accurately. - **Plot Mismatch**: The provided plot does not match the expected output (as per the attached image). Ensure that the plots in the README accurately represent the software's output to avoid confusion. - **Error in Error Analysis Code**: The error analysis code provided at the end does not run successfully. Please verify and correct the code examples to ensure they work as intended. - **Improve Image Alignment**: In the section titled "Example: Buck Circuit," the image alignment could be improved. Centering the image on a separate line would enhance the visual presentation. ## Documentation - **Enhance Index Page**: The [index page](https://mongibellili.github.io/QuantizedSystemSolver.jl/dev/) should include a clear statement of need, an overview of the package, and links to tutorials and API documentation. As the first point of contact, it should effectively introduce the software and guide users to relevant resources. - **Detailed Mathematical Descriptions**: Where possible, include detailed mathematical descriptions of the problems being solved, particularly in tutorials. This will aid users in understanding the underlying concepts and applying the software to their own problems. - **Improve Navigation**: The API reference and list of algorithms are only linked within the tutorial. Consider adding them to the navigation bar on the left for easier access. - The same applies to pages linked in the [Developer Guide](https://mongibellili.github.io/QuantizedSystemSolver.jl/dev/developerGuide/). - **Enhance High-Level Documentation**: The high-level documentation in the [Application Programming Interface](https://mongibellili.github.io/QuantizedSystemSolver.jl/dev/interface/#Application-Programming-Interface) section needs improvement. - Clearly explain how to use the `NLodeProblem` API. - Use proper formatting for function calls, similar to examples in [DFTK](https://docs.dftk.org/stable/api/) or [Gridap](https://gridap.github.io/Gridap.jl/stable/Io/). - **Purpose of Dependencies Page**: The [Dependencies](https://mongibellili.github.io/QuantizedSystemSolver.jl/dev/dependencies/#Dependencies) documentation page provides detailed descriptions of dependencies like *SymEngine*. While acknowledging dependencies is important, such detailed explanations may not be necessary. Consider summarizing or removing this section unless it serves a specific purpose. - **Consolidate Repeated Links**: The "Algorithm Extension" link is repeated in the [Developer Guide](https://mongibellili.github.io/QuantizedSystemSolver.jl/dev/developerGuide/). Please consolidate or remove redundant links to streamline navigation. - **Improve Extension Guides**: The `Problem` and `Solution` extension pages do not provide practical guidance on how to extend the software to new problems. Including simple, step-by-step examples would greatly benefit users looking to customize the software. - **Clarify `Taylor0` Documentation**: The [Taylor0](https://mongibellili.github.io/QuantizedSystemSolver.jl/dev/Taylor0) documentation needs improvement. The purpose and functionality of this component are not clearly explained. Providing more context and examples would be helpful. - **Organize Examples**: The [Examples](https://mongibellili.github.io/QuantizedSystemSolver.jl/dev/examples/) section could be organized into separate subpages to reduce complexity and improve user navigation. - **Automate Example Compilation**: Consider using [Literate.jl](https://github.com/fredrikekre/Literate.jl) to automatically compile examples. This ensures that code examples execute correctly and allows users to run them directly, which is a common practice in the Julia ecosystem. - **Exclude `build` Folder from Git**: The `build` folder within `docs` is included in the Git repository. Typically, this folder is generated automatically during continuous integration (CI) and should not be part of the repository. Excluding it can prevent unnecessary clutter and potential conflicts. ## API Documentation - **Add Doc strings**: Please add doc strings for all functions, such as those in `src/Utils/rootfinders/SimUtils.jl`. Comprehensive documentation enhances usability and helps users understand how to utilize the API effectively. ## Functionality - **Evaluation of Correctness**: While the software appears capable of solving ODE problems, it is challenging to assess the correctness of the implementation for more complex cases, such as stiff ODEs with events. Including a simple yet comprehensive example would greatly assist in validating the correctness and robustness of the solver. Are there any simple cases, where a comparison to exact solution might be possible? - **Example of Exponential Decay**: I tried solving the simple ODE $\dot{x}(t) = - x(t)$ with initial condition $x(0) = 1$ (whose analytical solution is $x(t) = e^{-t}$) which works correctly: ```julia using LinearAlgebra using QuantizedSystemSolver odeprob = NLodeProblem(quote name = (exp_decay) u = [1.0] du[1] = -u[1] end) sol = solve(odeprob, nmliqss2(), tspan, abstol=1e-10, reltol=1e-10) error_norm = norm(map(x -> exp(-x), sol.savedTimes[1]) - sol.savedVars[1]) # Output: 5.0644554862553414e-9 ``` Is it possible to include such a simple example that showcases the full capabilities of the software, i.e., with events, multiple solution variables which, at the same time, can be easily evaluated for correctness (like the exponential decay from above)? ## Tests - **Reference Results in Tests**: It would be beneficial if the test suite (e.g., `test/Tyson.jl`) included comparisons of computed results against reference solutions. This would help in detecting unintended changes in the software's output due to modifications, allowing developers to investigate and address issues promptly. - Notably, there are commented-out sections in tests like `test/adr.jl` that reference XLSX data. It is unclear why these comparisons are commented out. Improving the code quality of the tests by reactivating and ensuring these comparisons work correctly would enhance the reliability of the testing framework. - **Consistent Naming Conventions**: The files within the `test` folder currently have inconsistent naming conventions regarding the use of underscores and capitalization. Adopting a consistent naming scheme would improve organization and make the test suite easier to navigate and maintain. ## Community Guidelines - **Contribution Guidelines**: The project currently lacks clear guidelines for third parties who wish to contribute to the software, report issues, or seek support. Please provide detailed instructions. - These guidelines should be prominently featured in the README or a dedicated `CONTRIBUTING.md` file and referenced in the documentation. This will encourage community engagement and make it easier for users and developers to collaborate effectively. ## Paper ### Preamble - **Author Affiliation Correction**: The second author's affiliation should be corrected from "Kuleuven" to "KU Leuven" to accurately reflect the institution's official name. ### Paragraph 1 - **Clarity on Engineering Systems and Stiffness**: The sentence "The growing intricacy of contemporary engineering systems, typically reduced to stiff differential equations with events, poses a difficulty in digitally simulating them using traditional numerical integration techniques" may not be clear to non-specialists. - **Recommendation**: Provide a brief explanation of why contemporary engineering systems are often modeled as stiff ODEs with events. For instance: - **Stiffness Explanation**: Explain that stiffness arises in systems where there are rapidly changing and slowly changing components, making numerical simulation challenging due to the need for very small time steps to maintain stability. - **Engineering Systems Context**: Clarify what is meant by "contemporary engineering systems," perhaps by providing examples such as electrical circuits, mechanical systems with shocks, or chemical reactions with rapid kinetics. - **Precision in Describing the Quantized Approach**: The statement "It is an approach that builds the solution by updating the system variables independently as opposed to classic integration methods that update all the system variables every step" could be made more precise. - **Recommendation**: - **Mathematical Explanation**: Introduce mathematical formulations to explain the quantized state method (QSM). For example, describe how variables are updated only when they surpass certain thresholds (quantization levels), reducing the computational effort. - **Advantages Over Traditional Methods**: Highlight why the quantized approach is better suited for stiff ODEs compared to traditional numerical integration schemes, perhaps by discussing computational efficiency or improved stability. - **Formatting of Citations**: The last citation in this paragraph has an extraneous period. Ensure that all citations are correctly formatted according to the chosen citation style. ### Paragraph 2 - **Word Repetition Correction**: The phrase "Julia language Julia programming language" contains a repetition. - **Definition of an Event**: The term "event" is used without definition. - **Recommendation**: Provide a clear definition of an event in the context of ODEs. - **Consistency in Capitalization**: The term "Ordinary Differential Equations" should be consistently capitalized. - **Improvement in Sentence Structure**: The sentence "the QuantizedSystemSolver.jl is a solver that aims to efficiently solve" can be rephrased for clarity. - **Recommendation**: Maybe, replace it with "The `QuantizedSystemSolver.jl` package aims to efficiently solve...". ### Paragraph 3 - **Consistent Use of Capitalization**: The terms "Problem," "Algorithm," and "Solution" are capitalized without apparent reason. Think about using a monospaced font to indicate function or classes. - **Figure Reference**: The figure should be referenced within the text using a hyperlink or a figure number. - **Formatting of Code Elements**: The use of `NLodeProblem` should be formatted consistently. - **Clarification on Naming Conventions**: "NLodeProblem", I find the variable name strange, why is ODE written in lowercase? - **Recommendation**: Consider renaming it to `NLODEProblem` or `NonlinearODEProblem` for clarity and adherence to common naming conventions. - **Improvement of Code Examples**: The sample code could be more informative and aligned with standard Julia practices. - **Recommendation**: - **Use Real Julia Code**: Provide actual Julia code that can be executed, with comments explaining each part. - **Match with Mathematical Examples**: Ensure that the code corresponds to a mathematical example provided earlier in the text. - **Syntax Highlighting**: Use syntax highlighting for code snippets to improve readability (like in https://doi.org/10.21105/joss.04157). **Example**: ```julia # This is just a demonstration what I mean odeprob = NLodeProblem( quote, # quote "Simulation 1", # name (a=1, b=x->x^2), # parameters [x, t], # discrete and continous variables helper->beep(), # helper expressions [(x,t)->a*sin(x)*exp(-t)], # differential equations [ (t-1, execute_event()) # do something at t==1 ], # if-statments for events ) ``` - **Explanation of `quote ... end` Usage**: The use of `quote ... end` in the code is unconventional and may confuse readers. - **Recommendation**: Explain why this construct is used or consider adopting the standard method of defining functions and passing them as arguments, which is more familiar to Julia users. See also the other comment about the software design - **Consistency in Whitespace and Formatting**: - Add a whitespace after the assignment operator in `sol=`. - Ensure consistent use of whitespace after commas, e.g., `sol(0.0005,idxs=2)` vs. `tspan = (0.0, 0.001)`. - **Formatting of Citations**: Ensure that there is proper whitespace before citations, such as `SymEngine.jl (Team, 2015)`. - **Reference Author Listings**: For the three references in this paragraph, clarify how the authors are listed in the reference section to maintain consistency and proper attribution. Are the authors taken from the `LICENSE` file? - **Clarity and Precision in Conclusion**: - The sentence "In conclusion, the package offers extensive functionality to facilitate practical research tasks" is vague. - **Recommendation**: Be specific about the functionalities offered by the package, such as efficiently solving stiff ODEs with events using the quantized state method. - **Typographical Errors**: Correct the typo in "In Fact, Anyone" by removing the unnecessary capitalization. - **Simplify Complex Sentences**: The sentence "In conclusion, the package offers extensive functionality to facilitate practical research tasks, all while being lightweight and well documented to be easily used by researchers and students to efficiently model various dynamical systems with discontinuities, as well as further study and improve the newly developed QSS methods" is lengthy and complex. Please consider rewriting it. - **Grammar Corrections**: - Change "Anyone that has a different problem type" to "Anyone who has a different problem type." ### Figures - **Descriptive Caption**: The figure should have a descriptive caption that explains its content and relevance to the text. - **Formatting Within the Figure**: - In the "User Interface" node of Figure 1, ensure consistent use of whitespace and alignment for readability. ### References - **Missing DOI**: The reference "Pietro et al." is missing a DOI. ### General Remarks - **Statement of Need**: - **Recommendation**: Include a clear statement of need that illustrates the purpose of the software. This could involve presenting a challenging stiff ODE with events and demonstrating how the software effectively solves it. - **Comparison with Other Packages**: - **Recommendation**: - Include a comparison with traditional numerical integration methods to highlight the advantages of the quantized approach (maybe in a plot?). - Reference other software packages in this domain, even if they are not in Julia. - Maybe this one (https://github.com/CIFASIS/qss-solver) if it's relevant? - **Ongoing Research and Publications**: - **Recommendation**: Mention any ongoing research projects that utilize the software or recent scholarly publications enabled by it. This demonstrates the software's applicability and impact in the research community. - **Software Archiving for Reproducibility**: - **Recommendation**: Archive the software using a platform like Zenodo and include a citation with a DOI in the paper. This ensures long-term accessibility and facilitates reproducibility.
joshuaeh commented 1 week ago

This is good work: handling stiff systems efficiently is useful for all kinds of downstream tasks and hopefully researchers can use and extend these methods rather than implementing their own. @lamBOOO has done a great job, I have a few points as well mostly on the documentation and presentation side noted in a new issue:

https://github.com/mongibellili/QuantizedSystemSolver.jl/issues/20

diehlpk commented 1 week ago

@mongibellili could you please have a look at the comments.

mongibellili commented 1 week ago

@diehlpk sure, thank you.