JOSS review: Functionality and documentation

Hi @ben-cassese et al!

These are the rest of the comments I had about the submission. Thanks for addressing the first ones from here. Most of the submission looks great; from following the tutorials and testing the code everything seems to work properly. These are some minor comments and suggestions.

Functionality:

When running almost anything with squishyplanet (e.g. when using the .lightcurve() method) I get the following warning from jax:

/.../python3.12/site-packages/jax/_src/core.py:691: FutureWarning: unhashable type: <class 'jax._src.interpreters.partial_eval.DynamicJaxprTracer'>. Attempting to hash a tracer will lead to an error in a future JAX release. warnings.warn(

The warning does not produce an error or affect the functioning of the code as far as I can see, but I wonder if it's possible to change what is causing this warning or (probably more easily) to restrict the jaxversion in the requirements to avoid future incompatibility.

Documentation:

In the "Limb darkening profile check" of the Ilustrations notebook/tutorial, the variable radii is used but not previously defined; probably just a miss-spell for stellar_radii.
In the Create a transit lightcurve, the attribute projected_effective_r is wrongly called as effective_projected_r. Similarly, in the same notebook at one point the state dictionary is created with a key called projected_r which does not exist (should it be r or projected_effective_r instead?)
In the Fit a transit of an oblate planet tutorial, add a full reference or ADS link to Kipping 2013 when it is first mentioned.
At the bottom of the documentation there's a legend that reads "By Author name not set".

Software paper:

State of the field/statement of need: The newly added footnote properly comments on some differences between ellc and squishyplanet. One good thing to add/change in my opinion is a sentence or two on "advantages" that squishyplanet has over ellc or other software, or limitations that squishyplanet does not have but others do. I see for example that squishyplanet "allows for any [limb darkening] law that can be cast as a polynomial". I wonder if there are other advantages or other value that it adds (including non-functional matters like is it faster/more user friendly/easier to debug/lighter weight? etc.) that could be emphasized when software like e.g. ellc already exists. I believe that would be a more informative comparison to the community that, say, what back-end is it written on, when it comes to the community deciding to use the software for their specific cases.

Aside from the comments above, one general small comment would be that the new tutorial on Fitting a transit of an oblate planet (great addition!) could be more accessible for non-specialists by adding just a few words or references/links before certain cells, if anything just to explain what the code cell below is doing. Some examples:

Output of the 2nd cell: Just mentioning by name what the Greek letter mu represents in the paragraph above.
Output of 8th cell: Reminding the user that the first plot is the contour of the planet, since it looks different from the illustrations in the Illustrations section.
Output of the 10th cell: Explicitly mentioning what short and long cadences refer to; saying in the paragraph that 0,1,2,3 refer to polynomial orders (the labels "short 0, short 1" etc. were a bit confusing at first); adding a few words with something to the effect of "...in order to do X, let's look at the Lomb-Scargle periodogram" or something like that, and adding a link to Wikipedia or some paper about Lomb-Scargle.
Output of the 16th cell: A few words in the paragraph above or comments in the code box saying what changes made are fixing the oblateness to zero in the system and optimization; is there a rationale behind the bounds imposed in the minimization routine for each parameter?
Output of the 17th cell: Just a sentence being more explicit about how the models are being compared would be useful. Something like "To compare the models, let's use the Bayesian Information Criterion (BIC, link to reference/Wikipedia) and the Akaike Information Criterion (AIC, link to reference/Wikipedia)" or similar.
In general, adding short comments throughout the code just stating what a piece of code is doing would be useful (like it was done in the 18th cell of this tutorial, the first of the Nested Sampling Section).

Please reach out for any questions or clarifications about the comments above. The paper, software and documentation all look great! Looking forward to finish the review soon.

Thanks!

Review issue

Thanks for the additional comments! Here are my responses to the first two sections:

Functionality:

When running almost anything with squishyplanet (e.g. when using the .lightcurve() method) I get the following warning from jax:

Thanks for the catch! This looks like it's caused by a very recent change to jax. I've limited the jax and jaxlib versions to <0.4.29 to avoid the warning and hopefully avoid an error with future jax releases, but I might need to revisit the restriction later if inference packages in the jax ecosystem eventually outpace these versions. If you are using 0.4.29-0.4.31 and want to suppress the warning, warnings.simplefilter can help:

import warnings
with warnings.catch_warnings():
    warnings.simplefilter("ignore")
    lc = planet.lightcurve()

Documentation:

In the "Limb darkening profile check" of the Ilustrations notebook/tutorial, the variable radii is used but not previously defined; probably just a miss-spell for stellar_radii.

Thanks, fixed! Incorrectly left this after making changes to OblateSystem.limb_darkening_profile()

In the Create a transit lightcurve, the attribute projected_effective_r is wrongly called as effective_projected_r. Similarly, in the same notebook at one point the state dictionary is created with a key called projected_r which does not exist (should it be r or projected_effective_r instead?)

Ack whoops- I recently changed the way we handle 2D projections and just forgot to update this tutorial. Now instead of projected_r, which represented the major axis of the projected ellipse, we use projected_effective_r, which represents the radius of a circle with the equivalent area as the ellipse. That helps in a few ways: first, when fitting a transit, the old parametrization led to strong degeneracy between projected_r and projected_f. That's because the combination of both set the total area, which set the transit depth, which had a much stronger influence on the light curve than the slight deviations set by non-sphericity. In this new parameterization the degeneracy is much weaker. Second, we've now standardized the state attributes of OblateSystems parameterized by 3D or 2D geometries. That means a planet created under 3D now has a projected_effective_r and projected_theta that are computed during __init__ that can be directly applied to a 2D planet. I've simplified the cell you call out to reflect this and cut out the poly_to_parametric_helper import. The change is also currently commented out in the changelog and will be added when we release a new version to pypi.

In the Fit a transit of an oblate planet tutorial, add a full reference or ADS link to Kipping 2013 when it is first mentioned.

Added.

At the bottom of the documentation there's a legend that reads "By Author name not set".

Removed.

And here are some edits to the new tutorial:

Output of the 2nd cell: Just mentioning by name what the Greek letter mu represents in the paragraph above.

Added.

Output of 8th cell: Reminding the user that the first plot is the contour of the planet, since it looks different from the illustrations in the Illustrations section.

Added.

Output of the 10th cell: Explicitly mentioning what short and long cadences refer to; saying in the paragraph that 0,1,2,3 refer to polynomial orders (the labels "short 0, short 1" etc. were a bit confusing at first); adding a few words with something to the effect of "...in order to do X, let's look at the Lomb-Scargle periodogram" or something like that, and adding a link to Wikipedia or some paper about Lomb-Scargle.

Added an extra paragraph to the preceding cell, and changed the figure legend to be more descriptive.

Output of the 16th cell: A few words in the paragraph above or comments in the code box saying what changes made are fixing the oblateness to zero in the system and optimization; is there a rationale behind the bounds imposed in the minimization routine for each parameter?

Added an explanation for each bound as a comment to the code. They're all just wider than the priors used in the nested sampling section, but included to keep the minimizer from running off to something unphysical.

Output of the 17th cell: Just a sentence being more explicit about how the models are being compared would be useful. Something like "To compare the models, let's use the Bayesian Information Criterion (BIC, link to reference/Wikipedia) and the Akaike Information Criterion (AIC, link to reference/Wikipedia)" or similar.

Added the links to the preceding paragraph.

In general, adding short comments throughout the code just stating what a piece of code is doing would be useful (like it was done in the 18th cell of this tutorial, the first of the Nested Sampling Section).

I've sprinkled in more comments in most of the cells, but let me know if there are still areas that could use clarification.

Thanks for such specific edits on this! Appreciate the time/care on this review, the comments are definitely helping make the tutorials clearer 🙂

Last, about your comment on the state of the field section: respectfully, I'd rather not explicitly claim one package has an advantage over another in the way you suggest, since which to use really depends on your use case. If you are dealing with binary stars, anything to do with radial velocity, or want to model non-spherical stars, ellc is probably the way to go right now. If you want to simulate or fit a complex limb darkening law, or if you want to use any of the inference packages that rely on your model being written in jax (e.g. numpyro, jaxns, or blackjax), then squishyplanet would be more appropriate. The main reason for noting the backends is that the language choice can affect the available ecosystem for the rest of your analysis. We think that squishyplanet fills at least two niches that is not currently filled by any other software (extension and implementation of the Agol, Luger, and Foreman-Mackey 2020 to non-spherical planets, and implementation of a non-spherical planet transit simulator in jax), but that other software is more appropriate for other use cases. I hope that makes sense, let me know if you have other thoughts on this.

Also as a heads up, I'll be traveling next week, so no rush on responding to these!

Hi @ben-cassese ; thanks for the quick edits! I'll take a closer look. Following up in your last comment really quickly, I completely agree, I didn't mean claiming that a software was superior to another generally, I apologize if I implied otherwise. Your last comment is precisely what I wanted to convey: explicitly stating 1) which niches the software is filling (something made very clear by your two examples in parenthesis), and 2) a case or cases where your software could be more appropriate (like a complex limb darkening law or using a package that relies on the model written in jax).

As a reader who hasn't had to fit non-spherical transits, and who isn't very familiar with ellc, your last clarifications were very useful for understanding which use cases would benefit from squishyplanet. I think those explicit clarifications would be useful in the paper for similar readers, but I'm happy leaving it to your consideration. I will check that item off the checklist now.

I will quickly look into the other changes, and thanks for addressing the comments (even the ones for non-specialist readers).

Hi @ben-cassese ; I've looked through the changes and everything looks good. The bottom of the documentation still reads "By Author name not set" but that's just a small comment and does not affect the submission checklist. I'll finish checking the checklist items now and finish the review. This issue can be closed now. Great software!

Strange, I'll look more into the author name later, since it's gone on my local build but still being added to the readthedocs. Otherwise, thanks again for your review!

ben-cassese / squishyplanet

JOSS review: Functionality and documentation #13

Functionality:

Documentation: