JOSS requires sufficient testing for correctness.
It looks like SpectralDensities.jl tests for quite a few things, but SpiDy.jl only checks the code runs, not that the output is sensible.

It would be good to add a few tests that check output values, preferably some physically significant result, but reproducibility against a specific random seed is also okay.

Currently, all dynamics functions are called diffeqsolver, which, while to the point of what it does, might not be the clearest name for a new user. Furthermore, it makes the spin vs harmonic-oscillator difference very subtle (maybe this is good?).

We could rename these functions, so that they are more explicit, e.g. spin_dynamics(...) and harmonic_osc_dynamics(...) (or something like stochastic_dynamics(...) if we want to keep the same name for both).

To keep backwards compatibility, we could still define the diffeqsolver function names, so if current users update the library, their code keeps running (once we have proper releases and versioning, we can then drop the backwards compatibility at an appropriate major release).

This issue is used to trigger TagBot; feel free to unsubscribe.

If you haven't already, you should update your TagBot.yml to include issue comment triggers.
Please see this post on Discourse for instructions and more details.

If you'd like for me to do this for you, comment TagBot fix on this issue.
I'll open a PR within a few hours, please be patient!

Is your feature request related to a problem? Please describe.
There is no test in place to verify convergence. While this is not always needed, we should consider adding an optional parameter to turn it on.

Describe the solution you'd like
Add a parameter to diffeqsolver to turn on the convergence check. Use https://turinglang.org/MCMCDiagnosticTools.jl/stable/#MCMCDiagnosticTools.gewekediag maybe?

Describe alternatives you've considered
Make our own test, nothing fancy, something checking the first couple of moments?

Newer versions of DifferentialEquations.jl (and its dependency RecursiveArrayTools.jl) changed the way the solution can be accessed as an array, causing the example run files to error out. Indeed, when running the dynamic files, result in the following error:

% julia run/run_dynamics.jl
Starting...
ERROR: LoadError: TaskFailedException

    nested task error: AssertionError: length(I) == ndims(A.u)
    Stacktrace:
   ....

Apparently, the way the solution was used as an array, although it worked fine before, it was never officially supported (see: SciML/DifferentialEquations.jl#1021). The official way to access the solution from DifferentialEquations.jl is to do `Array(sol)'.

These are some comments to help the paper component of openjournals/joss-reviews#6263 be more compliant with a JOSS paper.

• Simon Horsley's ORCID of https://orcid.org/0000-0001-7242-7941 and Janet Anders's ORCID of https://orcid.org/0000-0002-9791-0363 seem to be empty. Having information in the ORCID is not required for publication, so this is fine, but if this was not intended they should double check their accounts.

• In

specifically

The package is written in pure Julia and we take advantage of the efficient DifferentialEquations.jl package...

please cite DifferentialEquations.jl. They provide a CITATION.bib:

@article{DifferentialEquations.jl-2017,
 author = {Rackauckas, Christopher and Nie, Qing},
 doi = {10.5334/jors.151},
 journal = {The Journal of Open Research Software},
 keywords = {Applied Mathematics},
 note = {Exported from https://app.dimensions.ai on 2019/05/05},
 number = {1},
 pages = {},
 title = {DifferentialEquations.jl – A Performant and Feature-Rich Ecosystem for Solving Differential Equations in Julia},
 url = {https://app.dimensions.ai/details/publication/pub.1085583166 and http://openresearchsoftware.metajnl.com/articles/10.5334/jors.151/galley/245/download/},
 volume = {5},
 year = {2017}
}

• All of the "Example" section should be removed. While I understand the motivation to include it, in the What should my paper contain? JOSS docs examples are not a requested paper section and we recommend adding all such useful examples to the documentation, if they are not already there.

• While the "Overview" section is not requested, if it is important to you to keep it you may, but it will need to be reduced in scope. As noted in the docs, the paper should include

A summary describing the high-level functionality and purpose of the software for a diverse, non-specialist audience.

where "non-specialist" means someone outside of your field. You could probably reduce this entire section down to a modified version of

though I appreciate that you might feel the need to give a bit more supporting detail, which is fine.

• We already discussed this in openjournals/joss-reviews#6263 (comment), but just to add it to the checklist, the glatthard2023harmonic/hogg2024stochastic reference should get updated.

Beyond that I think the paper is great and once this is done we can move forward with the versioning and archiving steps of publishing. 👍

Please let me know of any questions or concerns that you might have, and I'll be happy to try to help address them.

Currently, the bfield distribution argument takes a default value of nothing, i.e.

function bfield(N, Δt, J::GenericSD, noise::Noise, distro=nothing)
    if isnothing(distro)
        distro = Normal(0., 1/sqrt(Δt))
    end
   ...
end

which is then initialised to a Normal distribution as shown.

Apparently, Julia allows for default arguments to depend on the mandatory values. That is, something like this should work:

function bfield(N, Δt, J::GenericSD, noise::Noise, distro=Normal(0., 1/sqrt(Δt)))
   ...
end

If possible to do it this way, I think it would be an improvement as it would be clearer to the user what the default of nothing is actually doing.

Editorial notes as part of openjournals/joss-reviews#6263:

The current LICENSE file doesn't actually attribute any author with the copyright

It is recommended that some author(s) be added to the LICENSE (e.g. "SpiDy.jl Developers") along with the original development year.

From what I remember of the code you showed me on Friday, the constructor IsoCoupling(C) = C*I in the current version of the IsoCoupling type:

struct IsoCoupling{TT<:Real} <: Coupling
    C::TT
    IsoCoupling(C) = C*I
end

should not be there since it is not used for anything in the latest version. Indeed in the notebook where you were trying stuff in the office it shouldn't be there if I recall right.

Submissions to JOSS require some form of community guidelines for contributing to the code.
In general, this takes the form of a contributing file as explained here, and here is an example of the content of the file.

SpectralDensities.jl will also need one since it is covered in the JOSS publication.

If you want to go further (not required for JOSS) and add a code of conduct, PR and issue templates, github's community profile page is not bad place to start