langfzac comments

[tomkimpson]

Paper:

• The paper mentions that this code is meant to fill the space between full NR and simple post-Newtonian treatments. It would be good to include references to existing codes.

Documentation:

• The user is pointed to a file src/default_parameters.jl, which (I assume) should be src/system_parameters.jl.
• What is the run_speedy() function? I don't see it defined anywhere in the repo.
• The 3D PlotTrajectory() throws an error: UndefVarError: 'plt' not defined.
• StackedPlot throws an error: type Model has no field constant

Example Notebook:

• Supplying the NF=Float32 keyword to orbit() throws an error: No matching function wrapper was found! on Julia v1.9. However, it does work on v1.7.

• I think this is already known, but autodiff appears to be broken. It looks like in the dts_comments_1 branch, the notebook has an updated autodiff section, which is marked as "TODO".

Tests/Continuous Integration:

• I think the tests should be running on at least the Long-Term-Support version of Julia. Preferably, both LTS and the latest release. Currently, they only run on v1.7.

• Since the ability to use autodiff is a highlighted feature, there should be a relevant unit test for orbit().

• Consider using a doctest for the "how to run" example, and perhaps the example notebook. This would catch any issues with the example code, and make sure it stays up-to-date.
  Some of the test (e.g. test/orbit.jl) check for things like misspecified parameters via try-catch, but only check that any error is thrown. These could be made more explicit by testing for the particular error that is expected.
  General:

• It might be worth reworking the plotting functions in the Recipes framework from Plots.jl. It would remove the Plots dependency (which is historically a fairly heavy-weight one), and allow more flexibility for the user. Also, PlotTrajectory() does not display in Pluto notebooks, which would likely be fixed.

• I think there are some dependencies that could be dropped. For example, it doesn't look like Zygote or SciMLSensitivity are explicitly being used in any functions within the module. Others are BenchmarkTools, Enzyme, and ForwardDiff. It looks like Distributions is only used within the testing suite, which can have it's own Project.toml (like the docs).

[codecov-commenter]

Codecov Report

Merging #42 (dcc375e) into main (013537c) will not change coverage.
The diff coverage is 100.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff            @@
##              main       #42   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            6         6           
  Lines          330       332    +2     
=========================================
+ Hits           330       332    +2     

Impacted Files	Coverage Δ
src/orbit.jl	100.00% <ø> (ø)
src/universal_constants.jl	100.00% <ø> (ø)
src/useful_functions.jl	100.00% <ø> (ø)
src/initial_conditions.jl	100.00% <100.00%> (ø)
src/metric.jl	100.00% <100.00%> (ø)
src/timestepping.jl	100.00% <100.00%> (ø)

[tomkimpson]

Paper:

• The paper mentions that this code is meant to fill the space between full NR and simple post-Newtonian treatments. It would be good to include references to existing codes.
  • Yup nice idea. References have been added to the first paragraph of the "Statement of Need" section

Documentation:

• "How to run RelativisticDynamics.jl" section:
  • The user is pointed to a file src/default_parameters.jl, which (I assume) should be src/system_parameters.jl.
    • Good catch. Fixed
  • What is the run_speedy() function? I don't see it defined anywhere in the repo.
    • Old function/naming no longer used. Corrected.
• "Visualisation of the solution":
  • The 3D PlotTrajectory() throws an error: UndefVarError: 'plt' not defined.
  • StackedPlot throws an error: type Model has no field constant
    • In accordance with the comment below on dropping Plots.jl and using the recipes framework, both these functions have been dropped from the package. StackedPlot is just used in notebooks/plots_for_JOSS and now works correctly.

Example Notebook:

• Supplying the NF=Float32 keyword to orbit() throws an error: No matching function wrapper was found! on Julia v1.9. However, it does work on v1.7.

  • This was an error on the DE.jl side. Fixed by Fix RK4 Float32 time support and test a bunch more SciML/OrdinaryDiffEq.jl#1955. Unit tests now run on v1.9 and v1.6 (in accordance with comment below) and so NF=Float32 is tested by test/number_formats.jl

• I think this is already known, but autodiff appears to be broken. It looks like in the dts_comments_1 branch, the notebook has an updated autodiff section, which is marked as "TODO".

  • This is now fixed so that the autodiff works with Zygote.jl. See e.g. notebooks/demo.ipynb, and a test for autodif has been added to test/orbit.jl in accordance with below comment. Autodif was "broken" before since I was planning on switching to using Enzyme.jl rather than Zygote.jl , but - having explored this a bit more - in practice I have found Zygote to be much better established, tested, documented and just nicer to use.

Tests/Continuous Integration:

• I think the tests should be running on at least the Long-Term-Support version of Julia. Preferably, both LTS and the latest release. Currently, they only run on v1.7.
  • Nice idea. Fixed to run on v1.6 and v1.9
• Since the ability to use autodiff is a highlighted feature, there should be a relevant unit test for orbit().
  • A unit test has been added
• Consider using a doctest for the "how to run" example, and perhaps the example notebook. This would catch any issues with the example code, and make sure it stays up-to-date.
  • This is a cool idea, not something I was aware of, so thanks for bringing it up! Doctest using jldoctest has been added to docs/src/how_to_run.md and we also include the doctest in the unit testing via doctest(RelativisticDynamics) in test/runtests.jl. I don't feel it is needed for the notebook/demo.ipynb since the relevant code is already quantitatively tested and the notebook is more qualitative i.e. "how do the plots look?"
• Some of the test (e.g. test/orbit.jl) check for things like misspecified parameters via try-catch, but only check that any error is thrown. These could be made more explicit by testing for the particular error that is expected.
  • The "Bounds checks" unit test has been updated

General:

• It might be worth reworking the plotting functions in the Recipes framework from Plots.jl. It would remove the Plots dependency (which is historically a fairly heavy-weight one), and allow more flexibility for the user. Also, PlotTrajectory() does not display in Pluto notebooks, which would likely be fixed.
  • Really nice idea. The previous plotting functions have all been dropped along with the Plots.jl dependency. We now define a recipe in src/plotting.jl. A user can then plot their solution as:

using Plots 
solution,model = orbit();
plot(solution,model.constants.a)

This example is also given in notebooks/demo.ipynb

• I think there are some dependencies that could be dropped. For example, it doesn't look like Zygote or SciMLSensitivity are explicitly being used in any functions within the module. Others are BenchmarkTools, Enzyme, and ForwardDiff. It looks like Distributions is only used within the testing suite, which can have it's own Project.toml (like the docs).
  • Unused deps have been dropped and some moved just into testing suite. Zygote is now used in the main module (you are correct that it was not before!) so this remains a main dependency.

[langfzac]

Hi @tomkimpson, these changes look great! Only a few minor comments:

• The Visualisation docs page needs to be updated with the new plotting api.
• There's quite a bit of whitespace and commented-out code, most of which should be removed.
• I'm not sure what .github/workflows/orbit.jl is doing. It seems to just be a duplicate of the test/orbit.jl. Can it be removed?
• As per the review checklist, there could be more info on how best to contribute to the code, get help, etc. Perhaps a small "How to Contribute" section on the GitHub readme or in the docs.

Other than that, the following comments need not hold up the review -- I just wanted to point out somethings that may help.

• RE: doctests. Yes, I agree that you don't need a doctest for the example notebook. Although, what you could do is place the notebook code into a docs page and have it run when the docs are built (see Documenter.jl). This would 1) have the example in the docs and not a separate file and 2) catch any code thats out-of-date if you, say, make changes to the user interface.
• There are some places where I see speed-up opportunities. In MPD!, it looks like there are a bunch of arrays being allocated, which can slow down performance quite a bit since this is called many times in the ODE solver. For arrays that only exist in the function scope, you might try StaticArrays.jl. I'll defer to you on whether that make sense, since I don't know how much you've already looked at performance.

[tomkimpson]

Thanks @langfzac. All your minor comments have been addressed by the latest commit.

Re the optimisation comment, this is a good idea and definitely on the "to do" list, but something that I will defer for a v0.2.0.
I had looked at StaticArrays, having used them successfully in the past, but there are some issues with AD + Zygote: e.g. https://discourse.julialang.org/t/zygote-and-staticarrays/86176 . Optimising whilst persevering differentiability seems to be a little involved...

Once you are happy with the changes let me know and I'll merge this PR and return the discussion to the main JOSS thread.

[langfzac]

@tomkimpson. This looks good to me! I don't think I have any other comments. Happy for you to merge and I'll update my review checklist on the JOSS thread.