Updates for JOSS review 1

[dfm]

In this PR, I'll make the small suggested changes from @benjaminpope's JOSS review or link to issues/PRs tracking the larger changes. I'll update this comment as I go.

Unit tests:

• All passed in my default Python environment except for test_small_star and test_sky_coords. This was because I hadn't yet installed batman - if these are going to be in unit tests perhaps they should also be in dependencies. batman was included in the "test" extras but I've switched these tests to skip when batman isn't installed, making it an optional dependency to smooth issues like this. batman will still be installed an tested on CI.

Case Studies:

• Ran all the case studies locally on my machine by copying and pasting code. A couple worked fine, the others seemed to work ok but had very long MCMC chains and I didn't hang around to check their output. Could be neat to have these as notebooks in a notebooks/ directory on GitHub, as you just know people (read: me) are going to hack these as recipes to run on their own data. Tracked in Make links to notebooks prominent case-studies#16
• Just a note - check they all import matplotlib.pyplot as plt somewhere at the top? A couple of these don't and I needed to add this line. Perhaps the GitHub actions are importing this already? Tracked in Make imports and warning quashing less opaque case-studies#18
• It could also be nice to print run times on the case studies, so that prospective users know roughly how long each stage should take. Some of the MCMC chains and optimizations take a while to compute (of course), but as users are choosing between a few alternative packages, it will be helpful to know how this performs. Tracked in Print runtime summaries at the tops of notebooks case-studies#17
• There are many warnings: perhaps in the interests of a reader hoping to clone code there could be a warnings filter placed at the top. For example, I get a lot of (and this may be to do with my Python environment):
  • WARNING (theano.tensor.opt): Cannot construct a scalar test value from a test value with no size: InplaceDimShuffle{x,0}.0
  • WARNING (theano.tensor.opt): Cannot construct a scalar test value from a test value with no size: Elemwise{cos,no_inplace}.0
    Tracked in Make imports and warning quashing less opaque case-studies#18
• I'm actually a little uncertain about the boundary between Tutorial and Case Study. But I note that there is an excellent astrometry Tutorial (as long as the case studies and much longer than some of the tutorials), but no analogous Case Study doing astrometric fitting.
  Tracked in Move astrometry tutorial to case-studies and add a shorter astrometry tutorial #188
• Gaussian process models for stellar variability
  • Might like to add a sentence about why VI and MCMC retrieve such different posteriors?
    Tracked in Debug VI posterior in GP study case-studies#19
• Putting it all together:
  • Shouldn't this come last? Currently second in the list.
    Tracked in Re-name or re-order "Putting it all together" study case-studies#20
• RVs with multiple instruments
  • Mine crashed on multiprocessing sampling. Perhaps default in any notebook version should be 1 core?
    Tracked in Add documentation about multiprocessing gotchas (e.g. fork vs forkserver) #121

General paper comments:

• It is not unusual, though far from universal, for astronomy code in JOSS to be illustrated with a figure or two. (Around half of the recent papers I checked at random seem to have one). Perhaps the authors might like to include one in this paper, to illustrate the diverse capabilities of exoplanet. The multi-instrument light curve and RV fits from "Fitting light curves from multiple instruments" and "RVs with multiple instruments" respectively could go well as a multi-panel figure. (This is by no means a requirement for the paper to be accepted, just an opportunity).
  I have added a figure.
• The section on why autodiff is important in data analysis could perhaps do with some expansion. Not all astronomers are familiar with this, and perhaps another sentence or two selling and explaining the basic ideas of autodiff might help. (In my experience, many people do not realize it is different from finite differences). It might also do good to say explicitly whether it permits both forward and reverse mode autodiff, briefly highlight the advantages of Hamiltonian Monte Carlo, highlight the possibility of interoperability with neural network models, and so forth. I would even suggest putting a sentence in the Summary about the advantages of autodiff to hammer the point home. This is also not a required edit, but a suggestion to the authors on how to distinguish this work clearly.
  Tracked in Add a documentation page giving an intro to autodiff #187
• Citation formatting: fix brackets in "(based on the method used by the Stan project Carpenter et al.,2017.)" Similar problem with Theano, Theano Development Team, comma should be semicolon. Suggest semicolon before Carpenter would do? Fixed by re-wording. Should have citation to Jax. This was already cited.
• Citations - I know everyone has their homebrew exoplanet tool kit (which is why this project is so valuable) and that the Similar Tools section is is "far from a comprehensive list", but the paper could probably afford to cite more widely in this section. Perhaps a few other tools deserve a mention: for example Parviainen's PyExoTk or the Parviainen & Aigrain ldtk (I list these because they are the ones I have used myself; wide citation is to be encouraged, and perhaps the authors can think of others). Given the detached EB model, perhaps a citation to Irwin's eb or the PHOEBE or JKTEBOP projects?
  Done. I'm not including a citation to PyExoTk because it doesn't seem to have one or be licensed.
• "finite-difference gradients which can be subject to significant numerical errors and require 2N computations of a models with N free parameters" - might be good to cite and quantify this statement, while contrasting the scaling behaviour with dimensionality of proper autodiff.
  This will be addressed in Add a documentation page giving an intro to autodiff #187

[dfm]

@benjaminpope: One question regarding the recommended citations: What is the best reference for PyExoTk? All I can find is this GitHub repo and it doesn't even have a LICENSE so I'm hesitant to cite it.

[benjaminpope]

Good question. I recall using it years ago but maybe it was never published. Did it get eaten up by PyTransit or RoadRunner maybe? Can probably ignore this recommendation, it was just the first thing that came to mind from my own experience. ldtk at least is well-documented.

[dfm]

The remaining point that hasn't been addressed or given an issue is the suggestion of adding a figure to the paper. I'm somewhat hesitant since the paper is already much longer than a typical JOSS paper and I'm having trouble selecting a figure that would add to the paper meaningfully. But, I'm going to revisit that after addressing the other issues.

[ericagol]

The remaining point that hasn't been addressed or given an issue is the suggestion of adding a figure to the paper. I'm somewhat hesitant since the paper is already much longer than a typical JOSS paper and I'm having trouble selecting a figure that would add to the paper meaningfully. But, I'm going to revisit that after addressing the other issues.

How about a compilation of the case studies?

[dfm]

@ericagol: That's a good option, but I still feel like it would just be taking up space without adding much... but it probably wouldn't hurt!