spacetelescope / exovetter

Exoplanet vetting

https://exovetter.readthedocs.io

BSD 3-Clause "New" or "Revised" License

6 stars 5 forks source link

TST: Fix lightkurve 2.0.x compat, improve test header #50

Closed pllim closed 3 years ago

pllim commented 3 years ago

Looks like lightkurve finally changed their API.

Also add additional dependencies to test header.

TODO

[x] How to handle lightkurve/lightkurve#985 ?
[x] How to handle lightkurve/lightkurve#986 ?
[x] How to handle lightkurve/lightkurve#987 ?
[x] ~How to handle lightkurve/lightkurve#988 (dev job)?~ See #52

codecov[bot] commented 3 years ago

Codecov Report

Merging #50 (860b139) into master (ede22fb) will increase coverage by 0.03%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #50      +/-   ##
==========================================
+ Coverage   79.96%   80.00%   +0.03%     
==========================================
  Files          18       18              
  Lines        1188     1190       +2     
==========================================
+ Hits          950      952       +2     
  Misses        238      238

Impacted Files	Coverage Δ
conftest.py	`100.00% <100.00%> (ø)`
exovetter/lightkurve_utils.py	`72.72% <0.00%> (ø)`

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 ede22fb...860b139. Read the comment docs.

mustaric commented 3 years ago

As I was playing I realize that we still need to update vetters.ModShift to use time, flux, time_offset_str = \ lightkurve_utils.unpack_lk_version(lc, self.lc_name) This code was written so wecould go back and forth between new and old lightkurve, but apparently it wasn't implemented for ModShift. Do you want to do that as part of this pull request, or should I open a new one?

pllim commented 3 years ago

go back and forth between new and old lightkurve

I am 👎 on this. Given that corazon already uses lightkurve 2.0, I don't see a real need for such compatibility layer. It is only going to increase maintenance burden. Why do you still need lightkurve 1.0?

mustaric commented 3 years ago

This compatibility layer already exists. I wrote it so I could work with corazon and lightkurve at the same time. It seems to me the easiest path forward is to just make ModShift use the same layer and we can phase it out once we are sure everything is working correctly with lightkurve 2.0. I have no plans to long term support it, but since we already do, it is more work to take it out.

pllim commented 3 years ago

I don't have time to work on this today or the next few days, so feel free to fix modshift if you want. Thanks!

pllim commented 3 years ago

As per tag up today, I will wait till Susan applied her modshift changes before wrapping this up, to avoid conflict.

mustaric commented 3 years ago

I think this is ready to go. Merge when you are ready. I'd officially approve it, but I can't find the github button.

pllim commented 3 years ago

Thanks! I merged it. I'll open a separate issue for the dev job as it is pending reply from Geert.