search

openjournals / joss-reviews

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

[REVIEW]: GeometricIntegrators.jl: Geometric Numerical Integration in Julia #2954

Closed whedon closed 1 year ago

whedon commented 3 years ago

Submitting author: !--author-handle-->@michakraus<!--end-author-handle-- (Michael Kraus) Repository: https://github.com/JuliaGNI/GeometricIntegrators.jl Branch with paper.md (empty if default branch): Version: v0.7.0 Editor: !--editor-->@arfon<!--end-editor-- Reviewers: @ChrisRackauckas, @ranocha Archive: Pending

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

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

@ChrisRackauckas & @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 @VivianePons 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 @ChrisRackauckas

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @ranocha

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. @ChrisRackauckas, @ranocha 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

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

Can't find any papers to compile :-(

VivianePons commented 3 years ago

@whedon generate pdf from branch joss-paper

whedon commented 3 years ago 
Attempting PDF compilation from custom branch joss-paper. Reticulating splines etc...
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:

ranocha commented 3 years ago

Thanks for creating this nice package, @michakraus. The brief summary looks very interesting to me. If the package satisfies the expectations generated there, it will be a nice contribution to the community. Please find below some first comments after looking at the README and the manuscript. I'm looking forward to reading the documentation and trying out your package.

michakraus commented 3 years ago

Thanks for the nice words and the great suggestions! Except for the test of the README example I have implemented all of them. The SPARK reference should have hit the arXiv by now, but there were some delays. Should be online within the next one or two weeks, though. I will add the missing details to the reference asap.

Tom Tyranowski has implemented the first version of stochastic integrators. We are in the process of restructuring the library. We have already moved out several parts of the library into standalone packages (e.g., basis functions, quadrature rules, solvers, Runge-Kutta methods). In the next step we want to put several of the specialised integrators into separate packages, most and foremost StochasticIntegrators.jl, which we plan to publish separately.

ranocha commented 3 years ago

This is good work, @michakraus, and a nice contribution to the open source ecosystem. Here are some additional comments.

michakraus commented 3 years ago

Thanks a lot @ranocha for the thorough review. Working through your comments will improve the package and especially the documentation quite a bit. I will try to fix everything within next week, including the split of stochastic integrators. Let me comment on a few points right away, the rest I will just fix:

  • [x] It would be nice if the CI badge in the README.md linked directly to the CI GitHub action - which fails right now (commit 79282e0cc475433170de66367817aff14b3da432). It looks like CI is configured to fail if it fails on the nightly builds of Julia. It might be worth considering to change this behavior and show failing CI only if it fails on released stable versions of Julia.

Nightly builds are already allowed to fail without showing that CI failed. The Badge issue was fixed earlier today.

  • [ ] [...] Hence, I would like to know the anticipated scope of applications and the size of ODE systems? GeometricIntegrators.jl looks to be good for small ODEs. Can I also apply the integrators to spatial semidiscretizations of PDEs? If so, is it okay for moderate 1D problems or also for large 3D problems on a single workstation? It would be nice to mention this range of applications in the manuscript.

That's a good point. So far we have mostly targeted small to medium size ODE systems. We also use the library for PIC methods, but there the actual particle time steps decouple, i.e., one is again only solving small ODE systems. More recently, we thought a bit about FEM and DG methods, but we haven't performed any tests in that direction, yet. In principle, one can integrate arbitrary abstract arrays, warranted that a different linear solver is used for larger problems. We have a prototype wrapper around IterativeSolvers.jl, but that never got sufficient testing to make it into the repository.

  • [ ] When reading the manuscript, it is not really clear what kind of algorithms GeometricIntegrators.jl implements. It might be helpful to give some examples for each of the categories mentioned.

  • [ ] It would be interesting to know whether some of the methods implemented in GeometricIntegrators.jl are also available from other sources and mention differences and/or relative advantages/disadvantages. For example, some geometric numerical integrators are available as Fortran codes from the website of Ernst Hairer. A lot of Runge-Kutta methods are also available in Julia via DifferentialEquations.jl. What is the reason for having another implementation in Julia of these schemes? How do the methods of GeometricIntegrators compare to that?

Originally, the main purpose of the library was to implement our own integrators, that are not available elsewhere, and to use it both as a test bed and for providing reference implementation. We also added some standard methods for comparison / benchmarks. Recently, we have started to gather more geometric integrators (especially symplectic) from the literature and add them, in order to make GeometricIntegrators a truly comprehensive library for geometric numerical integration. Some of the specific Runge-Kutta methods (those with names) are also available elsewhere, but for certain classes (Gauss, Lobatto, Radau) we feature tableaus with an arbitrary number of stages, which you can rarely find elsewhere.

  • [ ] The docs for the modules "Basis Functions", "Linear Solvers", "Nonlinear Solvers", and "Quadratures" are nearly empty. It looks like these modules were moved to additional packages recently?

Yes, all of these sub-modules have been extracted. Seems I forgot to remove the docs...

  • [ ] Is adaptive time-stepping or some way to estimate an appropriate step size supported?

We currently have no special infrastructure for adaptive time-stepping. However, we have used adaptive sub-cycling in special-purpose integrators that are not part of the main library. Here, the standard infrastructure is used to specify "target time steps", i.e., the adaptive integrator computes a solution for every point in the equidistant time series, but in between in adapts as it wishes. Whenever an adaptive time step would step over a "target time step", it is reduced to hit that target. For geometric integrators that often is the only sensible thing to do (and for most practical applications as well). In all of this, the integrator has to take care of the sub-cycling and the rest of GeometricIntegrators doesn't really care about it. If for some reason you want to output the solution at the intermediate irregular time steps, this is relatively easily possible via the (still mostly undocumented) mid-level interface that is used to call the integrators. If I am not mistaken, DifferentialEquations interpolates the solutions of the adaptive steps to get a regular output, but that would destroy any structure, so it is important that the integrator actually integrates to each regular time step.

  • [ ] It is a bit confusing that an argument modified by a function is passed as last argument - the Julia standard convention is to pass modified arguments at first. What was the reason for this decision? It could be helpful to include a warning with an explanation in the docs.

I agree that this can be confusing. This convention was originally motivated by the usual notation of differential equations on jet bundles, where you have base space (time for ODEs) -> configuration space (the solution q) -> jet space (containing the derivatives, here v). We have thought about harmonising this with the Julia standard quite a bit, but we couldn't really come up with a satisfactory solution. We also thought about adopting the DifferentialEquations.jl convention. But in the end we found and still find the geometric convention most intuitive (at least if you live in that geometric integration bubble), although it clashes with the Julia convention. In any case you are right in that this deserves a more prominent mentioning in the docs!

  • [ ] Is there a specific reason for restricting the type of the right-hand sides to Functions? For example, I cannot use callable structs, which would be my first choice when dealing with differential equations with parameters (or encapsulating more complex right-hand sides).

Again a very good point. I'll look into that. Callable structs, however, seem to work well only for ODEs, where only one function for the vector field is specified. All the other equation types require several functions to be specified, thus callable structs would not be a good option. I guess the other equations types are yet another part of the library that needs a bit more documentation...

Hence, it looks like closures are the only way to deal with parameters right now.

In fact, each equation type supports an optional argument parameters which is supposed to be a NamedTuple. I guess that is not really (well) documented... it would look like

function my_function(t, x, v, p)
          @. v = p.a * x
      end

julia> ode = ODE(my_function, [1.0]; parameters=(a=1.0,))

Thanks again for your suggestions. Your help is really very much appreciated!

whedon commented 3 years ago

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

whedon commented 3 years ago

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

ranocha commented 3 years ago

I already posted some review comments above. When these are taken into account by modifying the package/docs/paper and @michakraus notifies me about this, I will finish the review.

VivianePons commented 3 years ago

Sorry, this is an automated message from whedon. I am not sure how to deactivate it.

@ChrisRackauckas Have you had time to start looking at the paper / software ? Thank you very much

ChrisRackauckas commented 3 years ago

Not yet, I had planned it for Feb 11.

VivianePons commented 3 years ago

No problem, thank you for the update!

VivianePons commented 3 years ago

Hey, just checking to see where we are on this (as a real person editor, not a robot!)

@michakraus have you worked on the different comments posted by @ranocha ?

@ChrisRackauckas have you had time to start the review?

Best

Viviane

michakraus commented 3 years ago

@VivianePons Thanks for checking in on this.

@michakraus have you worked on the different comments posted by @ranocha ?

I worked on most of the comments. However, in the process a few issues surfaced which are better fixed properly once and for all rather than quick and dirty. If things go well, I might finish this week, but depending on my workload it may also take a little longer...

Best, Michael

VivianePons commented 3 years ago

Good to hear! No rush over here, just making sure things are still moving ;)

ChrisRackauckas commented 3 years ago

Hey sorry, got far behind. I'll start by saying I think GeometricIntegrators.jl is a great package and I've dug around it before, but now am digging around with a reviewer's eye. But in general, even before the review eye, I would think it's publication worthy.

>Is adaptive time-stepping or some way to estimate an appropriate step size supported?

We currently have no special infrastructure for adaptive time-stepping. However, we have used adaptive sub-cycling in special-purpose integrators that are not part of the main library. Here, the standard infrastructure is used to specify "target time steps", i.e., the adaptive integrator computes a solution for every point in the equidistant time series, but in between in adapts as it wishes. Whenever an adaptive time step would step over a "target time step", it is reduced to hit that target. For geometric integrators that often is the only sensible thing to do (and for most practical applications as well).
In all of this, the integrator has to take care of the sub-cycling and the rest of GeometricIntegrators doesn't really care about it. If for some reason you want to output the solution at the intermediate irregular time steps, this is relatively easily possible via the (still mostly undocumented) mid-level interface that is used to call the integrators.
If I am not mistaken, DifferentialEquations interpolates the solutions of the adaptive steps to get a regular output, but that would destroy any structure, so it is important that the integrator actually integrates to each regular time step.

I think this is worth commenting on because it's probably lost to the eyes of someone who's not deep in the field. There really aren't good ways to do geometric integration with adaptive time stepping in the general case. If you dig through OrdinaryDiffEq.jl where the DifferentialEquations.jl symplectic integrators are, you see the documentation mentions it's all fixed time step. There are a few ways to adapt time by incorporating it into the Hamiltonians, but they are more research at this stage and I haven't seen them work well. But, OrdinaryDiffEq.jl is all based around adaptive time stepping algorithms, since that's "generally" what users want... except in the geometric case. So having geometric integrators in OrdinaryDiffEq.jl is rather suboptimal because it could skip a lot of the code complexity if one's focus is fixed time step geometric integrators. We had no reason to implement them in the SciML ecosystem because GeometricIntegrators.jl exists as high performance implementations of these fixed time step algorithms, and the GeometricIntegratorsDiffEq.jl bridge then serves the purpose of filling that part of the ecosystem. And indeed interpolation-based approaches of saveat breaks the structure, and so you really need a fixed time implementation. So it fills a very complementary role in the ecosystem and it does it really well.

Again a very good point. I'll look into that. Callable structs, however, seem to work well only for ODEs, where only one function for the vector field is specified. All the other equation types require several functions to be specified, thus callable structs would not be a good option.

It could make sense to give different callable structs to the two drift and diffusion functions of an SDE, each containing an appropriate cache. So I'm not quite convinced, but it's also not hard for the user to make the callable struct <:Function or wrap it in a (args...) -> f(args...) which would achieve the goal of making it a function with the callable.

All minor.

VivianePons commented 3 years ago

Hi @ChrisRackauckas thank you very much for these comments. Please also remember to look at the check list at the top of the issues to check off items that are ok already.

Best

Viviane

ChrisRackauckas commented 3 years ago

Updated. When those few points are hit I think that's enough for me to check the others.

VivianePons commented 3 years ago

Hi @michakraus have you had time to look at the comments?

michakraus commented 3 years ago

Hi @VivianePons, thanks for asking. I finished that big overhaul I mentioned before, but I still have to work through some of the comments. Although mostly minor, I'll probably only have time for that after easter. I am currently a bit busy with other stuff (administration, proposals, ...), but I'll try to do this asap.

VivianePons commented 3 years ago

No problem! I just wanted to make sure things were still in motion

VivianePons commented 3 years ago

Hi @michakraus just checking in as it's been a month, any timeframe on this?

michakraus commented 3 years ago

Hi @VivianePons, my apologies for the long silence. I had hardly any time for code related work in recent weeks. I will try to sort out the remaining points during this week.

VivianePons commented 3 years ago

No problem, happy to hear that you are still around!

VivianePons commented 3 years ago

Hi @michakraus any update?

michakraus commented 3 years ago

Hi @VivianePons, my apologies, this starts to become odd. Unfortunately, my last message still applies. I have been too busy with other things and hardly any time for coding. It seems I have the todo list of hydra: whenever I tick off one point there are at least two new ones. However, I have been working on the documentation and some other issues for the last few days, and hope to finish the last missing points next week. This time for real. In any case I will post a status report next week, stating which requests of the referees have been met and which are still open.

VivianePons commented 3 years ago

I understand, let me know when you are done. I'll be on leave myselst for the next 3 weeks and I can process things when I come back.

Best

Viviane

michakraus commented 3 years ago

I have been off the grid for about two weeks myself, but am now back to work. A short update: we just released GeometricIntegrators v0.9 which addresses many requests of the reviewers as well as many other changes. The main points still missing are

We will work on these (and some other) issues in the next few weeks with the goal of releasing GeometricIntegrators v1.0 as the reference version for the paper.

VivianePons commented 3 years ago

Great! Thank you for the update

VivianePons commented 2 years ago

Hi @michakraus where are you on this, did you have time to complete the work?

Thank you!

michakraus commented 2 years ago

Hi @VivianePons, sorry for the late reply. I was quite busy last week, but now, for the first time this year, I am free of any deadlines. I plan to spent most of my time until Christmas on software development. Unfortunately, in the last three months since my last status report (it's hard to comprehend that it has already been three months!), I did not have much time. Still, a few things that got done:

To make a long story short: I hope to be done before Christmas... it is about time!

arfon commented 2 years ago

Hi all, just checking in here. It's been nearly five months since the last update here...

VivianePons commented 2 years ago

Hi @michakraus where are we on this? Thanks!

michakraus commented 2 years ago

Dear @VivianePons and @arfon, apologies for the late reply. I have been off duty for the better part of the last 5~6 weeks.

We made some progress on the open issues, but not as much as I had hoped for. In the last months, we could not allocate a whole lot of resources to this project. However, most of the open issues need to be addressed in order to make progress on other projects or ours. So I have some hope that this will be taken care of soon. But we are currently fighting on several fronts, and it is hard to give an ETA. I will try to provide an update with a more concrete timeline before the end of June.

arfon commented 2 years ago

@editorialbot assign me as editor

:wave: folks. I'm going to be taking on this paper as @VivianePons is stepping down as a JOSS editor. Thanks for all of your help with JOSS @VivianePons!

editorialbot commented 2 years ago

Assigned! @arfon is now the editor

arfon commented 2 years ago

@michakraus - it's now nearly the end of July and we've not heard back from you in some time. Reviews in JOSS can sometimes take a while, but this submission looks to have been in review now for a very long time.

I propose we agree a firm deadline for you addressing the outstanding feedback (I propose one month from today), and if you're unable to meet this, we would proceed reject (or you could withdraw).

In this situation, a resubmission would be possible provided the feedback from the original review had been addressed.

michakraus commented 2 years ago

@arfon - apologies for being silent for so long. I very much agree that this has taken much longer than it should have. As mentioned before, boundary conditions have not been in favour of working on this project in the last months. Nonetheless, a few weeks back we merged a big PR leaving only one major problem open (refactoring solutions) in addition to some more documentation. I will be out of office for most of August, so it would be impossible to meet the suggested deadline. However, if we can agree on end of September there is a fair chance we can finish until then.

arfon commented 2 years ago

I will be out of office for most of August, so it would be impossible to meet the suggested deadline. However, if we can agree on end of September there is a fair chance we can finish until then.

OK, thanks for the update. Let's make September 30th the deadline for substantial (expected review-passing) updates.

arfon commented 2 years ago

Friendly reminder that we're expecting an update from you in a couple of weeks @michakraus.

michakraus commented 2 years ago

We haven't finished everything until today (the end of September), but we have made big progress. We mainly need some more work on the documentation. I will summarise the current state of affairs in two separate posts, replying to the referees' comments.

michakraus commented 2 years ago
  • [ ] As stated in the manuscript, the package "is designed to minimize overhead and maximize performance". However, there seem to be some caveats. For example, only LU decomposition seems to be implemented as linear solver, limiting the size of problems that can be solved. Hence, I would like to know the anticipated scope of applications and the size of ODE systems? GeometricIntegrators.jl looks to be good for small ODEs. Can I also apply the integrators to spatial semidiscretizations of PDEs? If so, is it okay for moderate 1D problems or also for large 3D problems on a single workstation? It would be nice to mention this range of applications in the manuscript.

In the manuscript as well as the documentation, we added the following section:

The library aims at providing efficient implementations of diverse algorithms in order to be able to perform simulations and benchmarks with millions or even billions of time steps that facilitate the study of the long-time behaviour of both numerical algorithms and dynamical systems. The current scope of applications is mainly small- to mid-size systems of differential equations, e.g., systems of ordinary differential equations or semidiscretisations of partial differential equations with a moderate number of degrees of freedom. It is envisaged that in the future GeometricIntegrators.jl will also be able to address larger problems, especially semidiscretisations of partial differential equations in higher dimensions. In particular, this requires interfaces to appropriate iterative and parallel linear solvers, which are still lacking.

  • [ ] It would be nice to have links to all papers mentioned in the documentation at Home/References.

We added a Bibliography section to the docs using DocumenterCitations. It's not beautiful but does the job.

  • [ ] When reading the manuscript, it is not really clear what kind of algorithms GeometricIntegrators.jl implements. It might be helpful to give some examples for each of the categories mentioned.

In the paper, we only list the various families of algorithms:

The implemented algorithms include explicit, implicit and partitioned Runge-Kutta methods, SPARK methods, splitting methods, symplectic methods and variational integrators.

We will add specific examples in the docs, but that is still WIP.

  • [ ] It would be interesting to know whether some of the methods implemented in GeometricIntegrators.jl are also available from other sources and mention differences and/or relative advantages/disadvantages. For example, some geometric numerical integrators are available as Fortran codes from the website of Ernst Hairer. A lot of Runge-Kutta methods are also available in Julia via DifferentialEquations.jl. What is the reason for having another implementation in Julia of these schemes? How do the methods of GeometricIntegrators compare to that?

I tried to account for that in the following reformulation:

GeometricIntegrators.jl provides a comprehensive library of geometric integration algorithms as well as some non-geometric algorithms. It collects native Julia implementations of many known methods under a unified interface. Once a problem is implemented in the GeometricIntegators framework, all of its algorithms can immediately be applied and their performance evaluated. This facilitates numerical experiments with a wide variety of algorithms, simplifies benchmarking, and makes it easy to identify the best algorithm for a given problem.

  • [x] The docs "Integrators/Splitting Methods" and "Integrators/Stochastic Integrators" are empty for v0.7.1.

Docs for Splitting Methods have been added. Stochastic Integrators were moved to a different package.

  • [x] The docs for the modules "Basis Functions", "Linear Solvers", "Nonlinear Solvers", and "Quadratures" are nearly empty. It looks like these modules were moved to additional packages recently?

All of these modules were moved to separate packages in the JuliaGNI organisation.

Many docstrings have been updated and extended, but this is an ongoing endeavour.

  • [ ] It would be nice if some standard structs used many times could be printed nicely in the REPL by overloading the appropriate methods of Base.show.

Dito. Base.show methods have been added for various data structures, but there is certainly more that could be done.

If you have specific structures in mind, please let us know.

  • [ ] Is adaptive time-stepping or some way to estimate an appropriate step size supported?

A paragraph has been added to the paper:

As most geometric integrators are not easily combined with time step adaption in a structure-preserving way, GeometricIntegrators.jl does not provide any general infrastructure for adaptive time stepping. Nonetheless, individual integrators can implement their own adaptivity strategies as long as they provide a solution at a predefined, equidistant series of time steps.

There's also a page "Developer/Adaptive Time Stepping" that explains this in some more detail but still lacks an example.

References to the various Runge-Kutta methods have been added to the docstrings of RungeKutta.jl. These docstrings can also be seen in the GeometricIntegrators docs under Tableaus -> Runge-Kutta. However, they do not appear in the Bibliography. We still need to add some references for the other methods:

This should be fixed, but the tutorial still needs to be extended a little bit further, also w.r.t. the next point.

  • [ ] It would be nice to see a few more examples for different problem types in the docs.

We started to work on this, but did not finish, yet.

  • [x] It looks like not all problem types and methods are compatible with each other, e.g. IDAE and TableauExplicitEuler. It might be good to mention something like this in the docs.

I started to build a list of all methods (including all possible combinations of e.g. Integrators and Tableaus), where entries have traits for the problem type they are applicable to. As soon as this is finished, we can auto-generate this overview for the docs.

  • [x] It is a bit confusing that an argument modified by a function is passed as last argument - the Julia standard convention is to pass modified arguments at first. What was the reason for this decision? It could be helpful to include a warning with an explanation in the docs.

This has been changed to comply with the Julia standard convention.

This may seem confusing on first sight, but is actually correct, cf. e.g. Marsden & West, 2001.

  • [x] Is there a specific reason for restricting the type of the right-hand sides to Functions? For example, I cannot use callable structs, which would be my first choice when dealing with differential equations with parameters (or encapsulating more complex right-hand sides). Hence, it looks like closures are the only way to deal with parameters right now.

It should now be possible everywhere to use Callable instead of just Function.

Parameters are dealt with in a very similar way as in DifferentialEquations.jl.

  • [x] Quoting [REVIEW]: GeometricIntegrators.jl: Geometric Numerical Integration in Julia #2954 (comment)

    In the next step we want to put several of the specialised integrators into separate packages, most and foremost StochasticIntegrators.jl, which we plan to publish separately.

    I'm not sure whether it would be better to make the splitting before publishing this work. Otherwise, the stochastic integrators would be in two papers - but that's not my decision, just a comment you might consider.

Meanwhile this has happened (some time ago).

  • [x] It would be good to have a top-level CONTRIBUTING file (mentioned also in the docs) so users see a link to it when they open a PR. It would also be good to note if you prefer users create a GitHub issue or use some other means for reporting problems.

A file `CONTRIBUTING.md has been added.

@ranocha Thanks once more for the helpful comments!

michakraus commented 2 years ago

Hey sorry, got far behind.

I guess I gave a new definition to "getting far behind". 

>Is adaptive time-stepping or some way to estimate an appropriate step size supported?
...

I think this is worth commenting on because it's probably lost to the eyes of someone who's not deep in the field.

  • [ ] I would say it would be good to add a note on this to the Statement of Need.

A paragraph has been added to the paper:

As most geometric integrators are not easily combined with time step adaption in a structure-preserving way, GeometricIntegrators.jl does not provide any general infrastructure for adaptive time stepping. Nonetheless, individual integrators can implement their own adaptivity strategies as long as they provide a solution at a predefined, equidistant series of time steps.

There's also a page "Developer/Adaptive Time Stepping" that explains this in some more detail but still lacks an example.

  • [ ] The developer documentation pages are empty.

Still mostly the case. Will be fixed asap.

This functionality has essentially been removed.

This feature and its docs has been disabled for now.

This has been moved to GeometricEquations.jl. The docstrings there are rather complete, but still need a little love to account for recent refactorings.

  • [ ] It looks like everything was made intentionally generically typed, but that doesn't seem to be tested? It might be good to throw a few Dual number tests in there just to see if everything works out like you'd expect.

Actually, some parts of the library (in particular GeometricEqations.jl) are generically typed, but in GeometricIntegrators itself, there is still a bit of refactoring needed to make this work with more than one or two integrators. Therefore we currently also have no tests for this.

  • [ ] I don't see a link to GeometricExamples in the docs? That might be good to mention.

GeometricExamples holds scripts for some runs for papers. It would be more appropriate to mention GeometricProblems, which holds actual test problems. References to GeometricProblems have been added in some places.

@ChrisRackauckas Thanks for the comments!

michakraus commented 2 years ago

@arfon To summarise, I think these are the open points, practically all concerning the documentation:

I expect we need another week or two to take care of all of these points.

arfon commented 2 years ago

Got it. Thanks for the update @michakraus!

danielskatz commented 2 years ago

👋 @michakraus - what is the path to resolving these open issues? It would be nice to finish this process in less than 2 years, which is coming up...

michakraus commented 2 years ago

I couldn't agree more that this should be finished as soon as possible. I have been working on the docs for the better part of my available time in the past few weeks. Unfortunately everything got delayed once more as I have been sick in between. I pretty much finished the GeometricEquations.jl docs and the revision of the tutorial. I will spend the next days adding more examples and extending the tutorial. This should be done by the end of the week.