JuliaAstro / Transits.jl

Flexible photometric transit curves with limb darkening

https://juliaastro.github.io/Transits.jl/dev/

MIT License

20 stars 6 forks source link

SimpleOrbit updates #22

Closed icweaver closed 3 years ago

icweaver commented 3 years ago

This PR updates the tests for SimpleOrbit, adds support for negative times, and re-defines when z is positive/negative to be in line with the new criteria introduced in ad996365887676ced6a90eb82a7bab1e72d623ea: https://github.com/JuliaAstro/Transits.jl/blob/eaa263bd64de46c33d8e108909413aeee2818356/src/Transits.jl#L82-L91

codecov[bot] commented 3 years ago

Codecov Report

Merging #22 (b82295f) into main (eaa263b) will increase coverage by 0.11%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #22      +/-   ##
==========================================
+ Coverage   90.61%   90.72%   +0.11%     
==========================================
  Files          12       12              
  Lines         959      960       +1     
==========================================
+ Hits          869      871       +2     
+ Misses         90       89       -1

Impacted Files	Coverage Δ
src/orbits/simple.jl	`100.00% <100.00%> (+3.12%)`	:arrow_up:
src/Transits.jl	`88.88% <0.00%> (+1.38%)`	:arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update eaa263b...b82295f. Read the comment docs.

icweaver commented 3 years ago

I'm currently setting z to 1 if the planet is on the half of its orbit nearest to us and 0 otherwise, but from a code re-use standpoint (and for secondary eclipses!) it would be nice if we could maintain the same condition that it is only 1 while in transit. I will take a look at this next

icweaver commented 3 years ago

Huh, just tried it out in exoplanet, and apparently the clipping is not just on our end: download

I was probably just getting too caught-up in trying to make everything look like our previous transit curves. Since exoplanet has a test comparing their simple orbit to a Keplerian one, I think we should be ok. I'll switch back to what we had before with the working version of SimpleOrbit we have now, ~and add in that test as well~ (after we settle on an API)

Further discussion at https://github.com/exoplanet-dev/exoplanet/pull/148

icweaver commented 3 years ago

I'm a bit confused- besides the mod vs %/rem change, everything is all good, right?

The reason why you're seeing the cutoff is because that's at exactly 1/4 the period (right?). Exoplanet basically says "you can't have a duration > period/2, but we don't stop you from trying" and I think keeping consistency with them there is good enough. I only wrote the SimpleOrbit because I wanted to get the limb darkening curves started and didn't want to make Keplerian orbits yet :)

Yea, I think that is reasonable and that things are looking good! Definitely interested to compare to our KeplerianOrbit curves =]

icweaver commented 3 years ago

Ok, things are looking green 👍🏾