exoplanet-dev / jaxoplanet

Astronomical time series analysis with JAX
https://jax.exoplanet.codes
MIT License
41 stars 12 forks source link

docs: keplerian orbits #109

Closed catrionamurray closed 9 months ago

catrionamurray commented 9 months ago
dfm commented 9 months ago

Some other comments:

  1. The remaining linter failures are all because of the somewhat draconian line length limit of 89:

    src/jaxoplanet/orbits/keplerian.py:38:90: E501 Line too long (115 > 89)
    src/jaxoplanet/orbits/keplerian.py:90:90: E501 Line too long (115 > 89)
    src/jaxoplanet/orbits/keplerian.py:109:90: E501 Line too long (94 > 89)
    src/jaxoplanet/orbits/keplerian.py:196:90: E501 Line too long (116 > 89)
    src/jaxoplanet/orbits/keplerian.py:199:90: E501 Line too long (92 > 89)
    src/jaxoplanet/orbits/keplerian.py:200:90: E501 Line too long (92 > 89)
    src/jaxoplanet/orbits/keplerian.py:201:90: E501 Line too long (100 > 89)
    src/jaxoplanet/orbits/keplerian.py:204:90: E501 Line too long (93 > 89)
    src/jaxoplanet/orbits/keplerian.py:206:90: E501 Line too long (110 > 89)
    src/jaxoplanet/orbits/keplerian.py:216:90: E501 Line too long (118 > 89)
    src/jaxoplanet/orbits/keplerian.py:217:90: E501 Line too long (95 > 89)
  2. The Read the Docs build is failing because this tutorial needs to be added to the table of contents. To do that, can you update the bottom of docs/index.md to include tutorials/Setting_up_an_Orbit.ipynb? (As an aside, I'd probably prefer to call it something like orbits.ipynb or intro_to_orbits.ipynb instead. Would that be ok?)

dfm commented 9 months ago

Another thought: maybe we should even call this the "Quick start" or "Getting started" tutorial?

Also @catrionamurray: can you add your info to .zenodo.json as part of this PR?

catrionamurray commented 9 months ago

Some other comments:

  1. The remaining linter failures are all because of the somewhat draconian line length limit of 89:
src/jaxoplanet/orbits/keplerian.py:38:90: E501 Line too long (115 > 89)
src/jaxoplanet/orbits/keplerian.py:90:90: E501 Line too long (115 > 89)
src/jaxoplanet/orbits/keplerian.py:109:90: E501 Line too long (94 > 89)
src/jaxoplanet/orbits/keplerian.py:196:90: E501 Line too long (116 > 89)
src/jaxoplanet/orbits/keplerian.py:199:90: E501 Line too long (92 > 89)
src/jaxoplanet/orbits/keplerian.py:200:90: E501 Line too long (92 > 89)
src/jaxoplanet/orbits/keplerian.py:201:90: E501 Line too long (100 > 89)
src/jaxoplanet/orbits/keplerian.py:204:90: E501 Line too long (93 > 89)
src/jaxoplanet/orbits/keplerian.py:206:90: E501 Line too long (110 > 89)
src/jaxoplanet/orbits/keplerian.py:216:90: E501 Line too long (118 > 89)
src/jaxoplanet/orbits/keplerian.py:217:90: E501 Line too long (95 > 89)
  1. The Read the Docs build is failing because this tutorial needs to be added to the table of contents. To do that, can you update the bottom of docs/index.md to include tutorials/Setting_up_an_Orbit.ipynb? (As an aside, I'd probably prefer to call it something like orbits.ipynb or intro_to_orbits.ipynb instead. Would that be ok?)

Updated both of these!

dfm commented 9 months ago

I think this looks great! The last thing that would be great to have would be an explicit test for that 4 pi^2 bug. Can you add that to tests/orbits/keplerian_test.py then let's merge!

dfm commented 9 months ago

Actually, I'm going to just merge this now and we can add the test later. @catrionamurray: would you be willing to open a new PR with the test?

catrionamurray commented 9 months ago

Actually, I'm going to just merge this now and we can add the test later. @catrionamurray: would you be willing to open a new PR with the test?

Yes will do!