Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added light travel time delay #60

Merged
merged 13 commits into from
Apr 28, 2020
Merged

Added light travel time delay #60

merged 13 commits into from
Apr 28, 2020

Conversation

rodluger
Copy link
Collaborator

Light travel time delay computed by a second-order Taylor expansion of the retarded position. The expansion works well, and I created a test for it. The only issues I'm currently having are the shapes: for a single planet and/or a scalar evaluation time, my method to get the planet position returns a different shape than yours.

Also, the time delay is computed relative to the origin of the coordinate system, so transits happen early and eclipses happen late. This causes the observed transit time to be different than the t0 provided by the user. One way around this is to define the light travel delay with respect to the point of conjunction with the observer. I can probably implement that but wanted to check with you first.

@dfm
Copy link
Member

dfm commented Sep 15, 2019

Thanks! I'll look at this more tomorrow. But, the test failures are new as of your changes (and they're all related to the Keplerian orbit that you edited) so can take a look to see if you can track them down?

@rodluger
Copy link
Collaborator Author

My bad -- there was a typo. Should be passing now.

@dfm
Copy link
Member

dfm commented Sep 16, 2019

For reference, here's my simple Python implementation of the iterative solver: https://gist.github.com/dfm/cfa2cbbae5b32e57536328564780e8f2

@rodluger
Copy link
Collaborator Author

@dfm wow, your style guide is draconian! What should I run on my end to make the style builds pass?

@dfm
Copy link
Member

dfm commented Oct 29, 2019

Haha! It should just be black, isort, and black_nbconvert. No worries about getting it to pass. I'm happy to do it! Some day I'll write developer docs.

@dfm
Copy link
Member

dfm commented Nov 12, 2019

If you come back to this, make sure that you rebase (or just take a fresh clone) because I'm trying to clean up the history and get rid of the huge files...

@rodluger
Copy link
Collaborator Author

OK will do.

@dfm
Copy link
Member

dfm commented Apr 9, 2020

I just re-visited this and I worked out the shape issues I think. Take a look and let me know what you think!

@dfm
Copy link
Member

dfm commented Apr 9, 2020

Also: @rodluger would you be able to add a few words to the docs about this? This could come in the form of a tutorial or just a longer docstring.

@rodluger
Copy link
Collaborator Author

OK, looking into this now.

@rodluger
Copy link
Collaborator Author

I added a really simple tutorial here. Sorry it's in *.ipynb form, but I did strip the output.

Things look mostly OK, except that light_delay=True doesn't play nice with use_in_transit=True in get_lightcurve. We'd have to change the contact points op to figure out the time of observed contact points. I could give that a shot, but I suspect it might be gnarly? Alternatively we could disable use_in_transit when the user wants to apply the light delay. What kind of performance loss will that incur?

@rodluger
Copy link
Collaborator Author

We could also just compute the maximum z-axis separation between the planet and the star and pad the transit window on the left by that amount to get a really conservative list of indices where transit might occur.

@dfm
Copy link
Member

dfm commented Apr 16, 2020

Great! I'd say we should just force use_in_transit to be False for now and revisit when the time comes.

@dfm
Copy link
Member

dfm commented Apr 17, 2020

I'm happy to merge this now that we're in the green if you're happy with it @rodluger. Thanks!

@dfm dfm merged commit 2c661cd into master Apr 28, 2020
@dfm dfm deleted the light_travel_time branch October 2, 2020 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants