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

JOSS manuscript review #323

Closed
PieterjanRobbe opened this issue Aug 29, 2024 · 4 comments
Closed

JOSS manuscript review #323

PieterjanRobbe opened this issue Aug 29, 2024 · 4 comments

Comments

@PieterjanRobbe
Copy link

PieterjanRobbe commented Aug 29, 2024

JOSS manuscript submission: openjournals/joss-reviews#7048

Congratulations on a very nice package!

I think the software is well-scoped, aiming at solving ODEs with filtering-based probabilistic numerics. It is nicely embedded in the existing DifferentialEquations.jl ecosystem and plays nicely with other Julia packages, e.g., Optimization.jl for parameter inference.

The documentation and tutorials seem very well written. The tutorials are straightforward and demonstrate the capabilities of the package.

The paper is also succinct and straightforward. I particularly liked the open and honest comparison to existing alternatives in other languages, e.g., probdiffeq in Python/JAX. Also, this package has already been used in a few papers listed in the manuscript.

I do have a couple of (small) suggestions:

(1) The code on the README.md doesn't evaluate properly, I think there's a comma missing after the first line in the statement

plot(sol.t, hcat(stds...)', color=["#CB3C33" "#389826" "#9558B2"]
     label=["std(u1(t))" "std(u2(t))"], xlabel="t", ylabel="standard-deviation")

Also, it would be great if this example would actually show the error bars, e.g., by solving the ODE with larger time steps?
(2) Some of the code in the tutorials doesn't work for me:

plot(sol, idxs=(3, 4))

but it does work with

plot(sol[:, 3], sol[:, 4])
  • In that same tutorial, a using BenchmarkTools is missing to be able to use the @btime macro.
  • In the Solving DAEs with Probabilistic Numerics tutorial, I need to add using LinearAlgebra to make sure Diagonal is defined.
  • Similarly, I need to add using OrdinaryDiffEq to make sure the Rodas4 solver is defined.
  • I also had issues compiling ModelingToolkit.jl on my julia installation (v1.9.3), so I couldn't execute the code below the modelingtoolkitize function here.
@nathanaelbosch
Copy link
Owner

nathanaelbosch commented Aug 30, 2024

Thank you very much for taking the time to review the package! I really appreciate your comments and your review and feedback helps a lot. I'll go through your suggestions one by one.

The code on the README.md doesn't evaluate properly, I think there's a comma missing after the first line in the statement

Good catch! Apparently a comma got lost in the last change. I will fix this asap.
EDIT: Fixed in 564495c

Also, it would be great if this example would actually show the error bars, e.g., by solving the ODE with larger time steps?

I also had this idea when I wrote the first README and in a previous version this was the case (see here). The problem I encountered is that with such a README, people expect to actually see the numerical error in their own examples, but in most cases the numerical error is much smaller than the solution. This lead to confusion when interacting with potential users, both on github and in person. This is why the current README now covers a (hopefully) more representative setting. What do you think, shoudl I keep the current one or rather revert to the earlier version? Depending on your feedback I would also be very happy to adjust the README example to a more informative one.

In the Second Order ODEs and Energy Preservation tutorial, the first plot does not show up for me

I just tried it with an up-to-date repository and there the plotting works. I can unfortunately not test the current release due to an installation error (#324) but I will test this again with the upcoming patch release. I will report back once I did this.

In that same tutorial, a using BenchmarkTools is missing to be able to use the @Btime macro.

You are totally right of course! That part of the doc is not Julia code but simply text so CI could not catch this. I will add a using BenchmarkTools line.
EDIT: Fixed in f610374

In the Solving DAEs with Probabilistic Numerics tutorial, I need to add using LinearAlgebra to make sure Diagonal is defined.

Good catch again! I will add LinearAlgebra to the imports.
EDIT: Fixed in f610374

Similarly, I need to add using OrdinaryDiffEq to make sure the Rodas4 solver is defined.

OrdinaryDiffEq.jl is part of the imports here.

I also had issues compiling ModelingToolkit.jl on my julia installation (v1.9.3), so I couldn't execute the code below the modelingtoolkitize function here.

I had no issues precompiling ModelingToolkit.jl with Julia 1.10.5, and also with 1.9.3. Though I also recommend to use ProbNumDiffEq.jl with Julia 1.10 and I set the compat entry accordingly in the Project.toml, as I do not test for older Julia versions, so I cannot guarantee that the code runs for older Julia versions.

Thanks again for all the helpful comments and for taking the time to go through the package and documentation so thoroughly! Let me know if anything else comes up.

@nathanaelbosch
Copy link
Owner

I just installed Julia 1.9.3 and I saw that I can not resolve the package dependencies on the current main branch, so it seems the most up-to-date version will not be compatible with Julia versions below 1.10 anymore. So I suppose that the environment in which you tested the code also used older package versions. Which is also a good thing, as you did not run into the current installation issue (#324), but I suppose that this also explains the failures in the plotting code possibly the ModelingToolkig compilation error, and I would expect that an update to Julia 1.10 resolves these. Does this address your issues to a satisfactory level? If not please let me know how I could resolve these best. Thanks again!

@PieterjanRobbe
Copy link
Author

Hi @nathanaelbosch, thanks for the quick response!

I tried again with a fresh install of 1.10.5, and it seems like indeed the plotting issue as well as the modelingtoolkit issue are now resolved, thanks!

As for the README, I understand your reasoning. I think you could just leave it as it is? I'll mark my review as complete.

@nathanaelbosch
Copy link
Owner

It's good to hear everything has been resolved. Thanks again for taking the time to review the paper and package!

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

No branches or pull requests

2 participants