Review for JOSS

[odunbar]

Hi @josemanuel22, I'm opening a thread to review your code/software paper for the JOSS review (openjournals/joss-reviews#7272)

Below I highlight items from the checklist, where I met issues, or had questions/comments if you do make changes to the codebase, please could reference this issue with the commits/ Pull requests that address them to help us track things. I'll check things off as I see them Thanks!

Review comments: Oct 7 2024

Overview

I'm afraid that it seems there is quite a lot of work to do here. Sadly the test pipelines failed for me (details below), and the examples wouldn't run due to a seemingly-long list of dependencies that aren't in the Project.toml. Please fix these first so I can complete the review. I am also a bit concerned about the package name - which is already used in a published package ISL.jl and I'm not sure about whether you can paste verbatim text in the software paper from a published source.

General

• Contribution and authorship: JOSS authorship includes, active project direction and other forms of non-code contributions (https://joss.readthedocs.io/en/latest/submitting.html#authorship) From the looks of it, this code is primarily a companion to the published work https://proceedings.mlr.press/v238/frutos24a.html . Have you contacted these authors to ask them to join your paper? - else are they OK with not being tagged in the submission else can you confirm below that they did not provide active project direction or other contributions?

• Data sharing: The data set for the Electricity time-series was not included. Ideally you would have a data downloader but if you want you could just add the DOI to the data both on the docs and in the example file? 10.24432/C58C86

• Reproducibility: I tried to run the example examples/Learning1D_distribution/benchmark_unimodal.jl. The Project.toml is incomplete. First it asked for CUDA.jl, then cuDNN.jl, then Zygote.... I stopped there. Please ensure this code can run from a completely fresh julia installation with the provided Project.toml (or give the examples their own Project.toml if there are heavy local dependencies), and I shall try again...

Functionality

• Functionality:
• Performance:
• Tests: Ran through, from scratch on julia 1.10.5.
  top level packages that got installed:

  [fbb218c0] BSON v0.3.9
  [336ed68f] CSV v0.10.14
  [052768ef] CUDA v5.5.2
  [35d6a980] ColorSchemes v3.26.0
  [5ae59095] Colors v0.12.11
  [a2441757] Coverage v1.6.1
  [a93c6f00] DataFrames v1.7.0
  [b4f34e82] Distances v0.10.11
  [31c24e10] Distributions v0.25.112
⌃ [587475ba] Flux v0.14.20
  [59287772] Formatting v0.4.3
  [cd3eb016] HTTP v1.10.8
  [09f84164] HypothesisTests v0.11.3
  [0b4ae3d3] ISL v0.1.0 `path-to-isl/ISL`
  [5f6e1e16] LocalCoverage v0.8.1
  [f1d291b0] MLUtils v0.4.4
  [d96e819e] Parameters v0.12.3
  [91a5bcdd] Plots v1.40.8
  [92933f4c] ProgressMeter v1.10.2
  [295af30f] Revise v3.6.0
  [90137ffa] StaticArrays v1.9.7
  [2913bbd2] StatsBase v0.34.3
  [ac1d9e8a] ThreadsX v0.1.12
  [c751599d] ToeplitzMatrices v0.8.4
  [a5390f91] ZipFile v0.10.1
  [02a925ec] cuDNN v1.4.0
  [37e2e46d] LinearAlgebra
  [de0858da] Printf
  [9a3f8284] Random
  [8dfed614] Test

I received many of the warnings:

WARNING: Method definition @test_experiments(LineNumberNode, Module, Any, Any) in module Main at /path-to-isl/ISL/examples/utils.jl:23 overwritten on the same line (check for duplicate calls to `include`).

perhaps you could fix this. Furthermore, the following tests failed for me (over several runthroughs actually), the solutions were not deterministic but failed each time.

Test learning Normal(4.0f0, 2.0f0): Test Failed at /path-to-isl/ISL/test/runtests.jl:156
  Expression: pvalue(HypothesisTests.ApproximateTwoSampleKSTest(validation_set, data)) > 0.01
   Evaluated: 8.914309000250083e-6 > 0.01
Test learning uniform distribution (-2,2): Test Failed at /path-to-isl/ISL/test/runtests.jl:179
  Expression: pvalue(HypothesisTests.ApproximateTwoSampleKSTest(validation_set, data)) > 0.01
   Evaluated: 0.0038958132550562085 > 0.01
Test learning modal auto_invariant_statistical_loss Normal(4.0f0, 2.0f0): Test Failed at /home/odunbar/Dropbox/Reviewer/Papers/Man24_JOSS/ISL/test/runtests.jl:249
  Expression: pvalue(HypothesisTests.ApproximateTwoSampleKSTest(validation_set, data)) > 0.01
   Evaluated: 0.002828941056680935 > 0.01
   

Please fix these.

Documentation

• A statement of need: The docs and readme are organized very much putting the repository / API at the front, whereas I think users (such as myself) would like to see a clear description of what it does and an example first. Note that saying it implements code from a paper is not equivalent to the description of the package. By and large what this does is learn from a set of samples of a probability distribution, to generate more samples
• Installation instructions: The list of dependencies is only partially in the Project.toml, other dependencies are not stated. please add or write a list of additional dependencies
• Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support Please create something clear for this

Software paper

• Summary & Statement of need: I have asked the editors the JOSS guidelines on copying these from published work. awaiting their response
• State of the field: This is completely missing, and is important, a cursory search turned up a variety of different diffusion model, transformer, Variational Auto-encoder and other such flavours of generative model packages in julia. (e.g. https://github.com/FluxML/model-zoo, https://github.com/LiorSinai/DenoisingDiffusion.jl, https://github.com/kailaix/ADCME.jl)
• Quality of writing: Waiting on request to editors...
• References: All references need DOIs - the Editors will ask for this too. Presumably you copied the bib from your paper in PMLR, note that it looks like the JOSS automated program also has issue with all the DOIs in references that you do not cite too. I'd recommend you remove unnecessary references too to avoid this. Also when making the citation a subject in L47, you should use @author, not [@author] so it says in Zaheer et al. (2017) - although this is a little pedantic!

Miscellaneous

• Name-clash One other strange comment is that there is already a registered package ISL.jl in the Julia registry. This means that you will not be able to ever register this package with the same name. The Julia convention is to name packages with descriptive names, and they must end in .jl. I expect this is the last chance to change the name.
  In any case, you need to at least remove the references like "Welcome to ISL.jl" that you have on the docs landing page.