search

ericagol / NbodyGradient.jl

N-body integrator computes derivatives with respect to initial conditions for TTVs, RV, Photodynamics & more
MIT License
20 stars 9 forks source link

WIP: Radius, impact parameter, obtain_orbital_elements #92

Open Tusay opened 1 year ago

Tusay commented 1 year ago

Most significant changes are adding radius to the element matrix and more finely calculating impact parameter in transit timing.

Also changed get_orbital_elements to obtain_orbital_elemtns in convert.jl and added that to be exported in the main code.

ericagol commented 1 year ago

Hi Nick & Zach - @Tusay @langfzac I'm still looking this over, but have a couple of questions/comments: 1). I think there was an error in the transit criterion, which still needs fixing. Do you know if your changes fixed this? 2). What is the main purpose of these changes? 3). Have you added a test (or tests) to see whether things are working as expected? -Eric

langfzac commented 1 year ago

This is definitely still a work-in-progress (I just changed the title accordingly). The main thing is to add the radii as an optional parameter of the model and fix up the transit criterion (@Tusay we should change this so that the radii are given as a parameter of the TransitTiming type, rather than the State).

Tusay commented 1 year ago

Eric & Zach,

If I remember correctly, I had changed the transit criteria to make it more discriminating and generalizable to planet-planet occultations.

As the code is written, transits are set to trigger on vector sign change, and the position being a fraction of the orbital distance toward the observer (likely to exclude secondary transits), and the orbital distance less than rstar (which was seemingly arbitrarily set to 1e12).

I changed it to trigger on vector sign change, and the position of the transiting/occulting planet being more toward the observer than the occulted body, and the orbital distance less than that same arbitrary rstar value.

I had played around with changing that to a calculated impact parameter, but I think that was giving me strange results. I would still like to play with it more though. I think the problem is that the planets are just so small compared to how far they move in a given timestep. So, I think I had intended to try just making that impact parameter a factor of what we want to see, but I didn't get around to it.

Again, the purpose for these changes is to more reliably capture planet-planet occultations. The original trigger may miss some occultations because the trigger should be relative between the planet and the occulted body, which may or may not be the star.

Hope that clears up what I was going for.

-Nick

Nick Tusay Graduate Student Research Fellow Penn State University Department of Astronomy & Astrophysics

[cid:40faf81a-0b2b-431d-ad60-c1dbef9c0624]

From: Eric Agol @.> Sent: Friday, November 18, 2022 2:02 PM To: ericagol/NbodyGradient.jl @.> Cc: Tusay, Nicholas @.>; Mention @.> Subject: Re: [ericagol/NbodyGradient.jl] Radius, impact parameter, obtain_orbital_elements (PR #92)

Hi Nick & Zach - @Tusayhttps://nam10.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FTusay&data=05%7C01%7Ctusay%40psu.edu%7Cdb00e550e46342f9edf108dac9976188%7C7cf48d453ddb4389a9c1c115526eb52e%7C0%7C0%7C638043949265845356%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=iuF0eQ0v28Z5ltkL5H6kQXSEnv2d5AoX0Y7RiytC4F8%3D&reserved=0 @langfzachttps://nam10.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Flangfzac&data=05%7C01%7Ctusay%40psu.edu%7Cdb00e550e46342f9edf108dac9976188%7C7cf48d453ddb4389a9c1c115526eb52e%7C0%7C0%7C638043949265845356%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=XPPxQf%2BTmTgEj%2B5BF1sQMNDpex6pDNzFlxRr9QlE9Y4%3D&reserved=0 I'm still looking this over, but have a couple of questions/comments: 1). I think there was an error in the transit criterion, which still needs fixing. Do you know if your changes fixed this? 2). What is the main purpose of these changes? 3). Have you added a test (or tests) to see whether things are working as expected? -Eric

— Reply to this email directly, view it on GitHubhttps://nam10.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fericagol%2FNbodyGradient.jl%2Fpull%2F92%23issuecomment-1320416724&data=05%7C01%7Ctusay%40psu.edu%7Cdb00e550e46342f9edf108dac9976188%7C7cf48d453ddb4389a9c1c115526eb52e%7C0%7C0%7C638043949265845356%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=WJliWKR9RpgpV4C%2BxBCcbbXQVJKUPbps13rogT7QVI4%3D&reserved=0, or unsubscribehttps://nam10.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAQVE4XPWFUD5SIUILNMD37LWI7HCXANCNFSM6AAAAAAR55XJHQ&data=05%7C01%7Ctusay%40psu.edu%7Cdb00e550e46342f9edf108dac9976188%7C7cf48d453ddb4389a9c1c115526eb52e%7C0%7C0%7C638043949265845356%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=IHmtUGkWP4TogmjN%2FWJ53ZiDkEIXf9Pk3pyeParKxqA%3D&reserved=0. You are receiving this because you were mentioned.Message ID: @.***>

Tusay commented 1 year ago

This is definitely still a work-in-progress (I just changed the title accordingly). The main thing is to add the radii as an optional parameter of the model and fix up the transit criterion (@Tusay we should change this so that the radii are given as a parameter of the TransitTiming type, rather than the State).

I think that makes sense. So long as it's optional and retrievable. It really only matters to the transits and they are static input values.

ericagol commented 1 year ago

@Tusay Thanks for the summary! The rstar is extremely arbitrary, and needs to be looked into further.