[JOSS Review] Documentation

[ygrange]

Since I've got a few comments on the documentation, I decided to put the comments in here so that we can have a topical discussion on that too.

I mentioned this in the README as well, but if I install starmatrix clean, and run it without a config file I get an answer. I think that means you have some set of sane default values.

It would be great, especially for newcomers to the field, to document those. I did notice that the default config file that is generated actually has defaults so it may just be as simple as copy-pasting from there. If it makes sense you could even add some extra sentence describing the standard set (think of something like "default values represent the most popular IMF, with solar environment type metallicity, "etc.

readthedocs

https://starmatrix.readthedocs.io/en/latest/usage.html
custom_params = { ... } I assume this is a dict containing the same parameters as are documented in the configuration page: https://starmatrix.readthedocs.io/en/latest/configuration.html . May be nice to explicitly say this and add a link.

About the starmatrix utility functions:
You mention those but as far as I can see there is no API documentation on readthedocs. What may be useful is to give high-level API documentation in the readthedocs (e.g. make a list of the package contents and tell what is what, so basically this list with one sentence per item:

    abundances
    cli
    constants
    dtds
    elements
    functions
    imfs
    matrix
    model
    settings
    supernovae
    tests (package)

And then refer to the python help for documentation of the functions/objects underneath.

Also: It is mentioned here and there that one can add their own models by subclassing something, but I would expect the docs to actually tell me what to subclass with a small example.

Python docstrings

I noticed the docstrings do not all explain each of the parameters explicitly, in some cases it is not really clear to me what the parameter is supposed to mean. If I look at for instance the function class (which I assume is going to be the one most people will want to develop against, most of the functions take the same set of parameters so this should not be a huge effort.

For example

    imf_binary_primary(m, imf, binary_fraction=0.15)
        Initial mass function for primary stars of binary systems
        Integrated between  m' and m'' using Newton-Cotes
        Returns 0 unless m is in (1.5, 16)

I guess imf is an imf object type, or a string representing an imported one? And m is a mass, but then you take the first and second derivative? So maybe m is a function defining the mass distribution?

I would propose a format like (just guessing here):

imf_binary_primary(m, imf, binary_fraction=0.15)
Initial mass function for primary stars of binary systems Integrated between  m' and m'' using Newton-Cotes. 
arguments: 
m: int, mass in solar masses for which to define the number of stars
imf: staramtrix.imfs, imf to compute the mass for
returns: the number of stars between m' and m'', or 0 if m is not in (1.5, 16)

(you could use an existing docstring standard as well of course)

[ygrange]

Relates to: openjournals/joss-reviews#4461

[xuanxu]

I've expanded the list of modules with a short description and a link to the code.

[xuanxu]

It is mentioned here and there that one can add their own models by subclassing something, but I would expect the docs to actually tell me what to subclass with a small example.

I noticed the docstrings do not all explain each of the parameters explicitly, in some cases it is not really clear to me what the parameter is supposed to mean. If I look at for instance the function class (which I assume is going to be the one most people will want to develop against, most of the functions take the same set of parameters so this should not be a huge effort.

@ygrange: I love these two recommendations. I'll definitely add both examples on how to run custom models subclassing IMF or Abundances, and improve docstrings adding info on the parameters.

But I don't have many spare time right now because I'm going through a mix of a busy work schedule and short family trips so depending on how needed you consider this recommendations for the review I'll try to add them as soon as possible (if these are blocking issues) or after the summer for the next release (if you consider them nice-to-haves).

[ygrange]

That is a fair point. I think in principle it does not block usage from the code (the level of documentation is fine for the casual user, it could just improve for the advanced user). Maybe you can leave this issue open till whenever you do the work and I'll proceed to check the "API documentation" box in the review checklist (i.e. will not make this blocking but am still interested in your changes :) ).

[xuanxu]

👍