exoplanet-dev / exoplanet

Fast & scalable MCMC for all your exoplanet needs!
https://docs.exoplanet.codes
MIT License
206 stars 52 forks source link

Fixing `SimpleTransitOrbit` duration #148

Closed christinahedges closed 3 years ago

christinahedges commented 3 years ago

This PR fixes #147.

The issue is that the transit from SimpleTransitOrbit does not match the transit from KeplerianOrbit when given reasonable parameters.

This PR also fixes:

dfm commented 3 years ago

I don't think that this is quite what we want. The 0.5 was right and I don't think we want to have a as an input parameter. The whole point is that these shouldn't be Keplerian orbits. So let me try to explain what I was thinking:

In the current implementation, we define the orbit as:

x = v * dt
y = b

where v is the speed and dt is the time since the center of the transit. This all good, but the problem is that the definition of v is currently given by the triangle:

______
\    |
1 \  | b
    \|

Where Instead, we want it to be:

______
\    |
h \  | b
    \|

where h = 1 + ror so that the transit starts when ingress starts and not in the middle of ingress.

So: I think that we should accept ror as a parameter (perhaps with a default value of 0.0 for backwards compatibility) and then compute:

x2 = r_star**2 * ((1 + ror) ** 2 - b ** 2)
speed = 2 * sqrt(x2) / duration

in the init for SimpleTransitOribt.

christinahedges commented 3 years ago

Hey @dfm thanks for the feedback this is great!

That definitely fixes the duration issue, I've implemented it and it looks good!

My fix with this line was to make it so z is 1 effectively up to a ror of 1. Before, the planet was at z = -1 before the center of the planet hits the limb of the star, which I believe means the planet is behind the star, causing the transit shape I was seeing: image

Alternatively, setting that line to z = tt.ones_like(x)

would mean the planet is always in front of the planet, and then we wouldn't need to prescribe ror. However, I assume there is there a reason z behaves this way?

For the a parameter, I took that out, you're totally right. It might be nice to be able to set something similar ("separation at closest approach"?) because it's a nice user feature, but that's just a suggestion.

dfm commented 3 years ago

Awesome - thanks!

Yeah - the z stuff is so that it doesn't try to pass in front of the star during occultation. I think it should work properly with the version you have implemented now though right?

dfm commented 3 years ago

(PS don't worry about the failing tutorials workflow it's not set up to work with PRs - something I need to fix!)

christinahedges commented 3 years ago

I implemented the suggested change, and yes now the z definitely works!

dfm commented 3 years ago

Thanks Christina!