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

Fixing error in integrated term #56

Merged
merged 4 commits into from
Aug 6, 2019
Merged

Fixing error in integrated term #56

merged 4 commits into from
Aug 6, 2019

Conversation

dfm
Copy link
Member

@dfm dfm commented Aug 6, 2019

@ericagol: there was a missing cosh in the derivation of the integrated term for tau < Delta in the manuscript (it was right in the code!), but I found it when deriving the correction for the diagonal of the celerite matrix (which hadn't been implemented correctly before).

@dfm dfm requested a review from ericagol August 6, 2019 15:07
Copy link
Collaborator

@ericagol ericagol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I worked through the algebra, and there was definitely a mistake in my computations (probably in a Mathematica notebook which I can't access right now since my license just expired). I now agree with your equations (C37) and (C38), and it's reassuring that these make more sense based on the symmetry of the equations. Sorry about the error, and I hope it didn't waste too much of your time. Have you tried out the new diagonal term? At some point I would like to take the Fourier transform of the complex kernel to check that it agrees (although it must!).

One thing that might be good would be to change the C1 and C2 in the lines below to match the definition in the paper:

https://github.com/dfm/exoplanet/blob/a86327a0690a0ccd41b5972ab76537acceb1e5bd/exoplanet/gp/terms.py#L363-L364

Nice work!

-Eric

@dfm
Copy link
Member Author

dfm commented Aug 6, 2019

Thanks! It was no problem - just wanted to make sure that I hadn't missed something.

I've updated those lines so that they are consistent with the document now. Thanks!

Copy link
Collaborator

@ericagol ericagol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks!

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