Fixing SimpleTransitOrbit duration

[christinahedges]

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:

• SimpleTransitOrbit has a default duration of None, which can not be initialized. Now raises a ValueError.
• SimpleTransitOrbit did not accept a semimajor axis parameter. Now accepts a as an input, and will calculate duration from that value.
• There is now a test to check that SimpleTransitOrbit matches KeplarianOrbit at long period and reasonable b.

[dfm]

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]

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:

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]

period is also required. How about we change the function signature to:

def __init__(self, *, period, duration, t0=0.0, b=0.0, r_star=1.0, ror=0.0):

and remove this check?

[christinahedges]

Definitely happy to do this, I'll just have to update the tests because they're specified without kwarg names e.g SimpleTransitOrbit(period, t0, b, r_star, duration). Are you ok with me doing that?

[dfm]

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]

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

[christinahedges]

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

[dfm]

Thanks Christina!