documentation and argument names for flux_pdf

[JostMigenda]

[As part of the JOSS review.]

I’m overall impressed by the good documentation across the whole code base; even for internal functions that users of FIRESONG will never interact with directly. Well done!

I just noticed two very minor issues:

• In the docstring for flux_pdf, the description of LumMin/LumMax and logFMin/logFMax says things like "Max luminosity to consider, in UNITS". Could you replace that with the correct units?
• In the docstrings for flux_pdf and firesong_simulation, the argument timescale is described as "timescale (bool, optional, default=1000): Timescale in seconds" — I suspect that should be an int, not a bool?

---

While I’m talking about minor issues regarding the arguments to flux_pdf, I may as well expand on this point that @rafaelab made:

Maybe this is me being picky, but I found it annoying that the keyword "Evolution" in firesong_simulation() is capitalised when all the other keywords are not. I got this wrong twice when trying to run a simple test without copy-pasting. The same goes for "Transient".

There are a few additional inconsistent argument names in flux_pdf:

• zmin/zmax and emin/emax, but LumMin/LumMax
• nFluxBins vs. nLbins ("bins" being uppercase in the first, but lowercase in the second one)
• "flux" is written out in nFluxBins and fluxnorm but abbreviated in logFMin/logFMax

This obviously shouldn’t block acceptance of the JOSS paper, but it would remove a tiny pain point when using the code. (Like Rafael, I too have been tripped up by this once or twice during my testing.) Of course, this is effectively a backwards-incompatible change to the API; so if you do decide do rename these arguments, I completely understand if you do it slowly and carefully so you don’t break all analysis scripts that use FIRESONG across multiple experiments. 😉

[ChrisCFTung]

1. Doc string of flux_pdf has been addressed in #65
2. Indeed the inconsistency of the options is an issue for user experience, but considering the impact that can be brought by changing those, I think it's a better choice to keep it the same

[JostMigenda]

That's fair. Of course, if there's ever a FIRESONG v2.0 which breaks backwards compatibility anyway, feel free to revisit this …