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

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

Merged
merged 10 commits into from
Mar 4, 2021

Conversation

@pllim pllim added bug Something isn't working testing labels Feb 26, 2021
@pllim pllim changed the title TST: Fix lightkurve import, improve test header TST: Fix lightkurve 2.0.x compat, improve test header Feb 26, 2021
@codecov
Copy link

codecov bot commented Feb 26, 2021

Codecov Report

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

Impacted file tree graph

@@            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.

flat = lc.flatten(window_length=81)
flat.flux = flat.flux - 1.0
flat.flux = flat.flux.value - 1.0
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am not sure if this is completely correct fix, though it does make the test pass again.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess ideally we would like to make sure that flat.flux persists as a quantiity instead of as a float. So I think this line should be
from astropy import units as u
flat.flux = flat.flux.value - 1.0) * u.electron/u.electron
The units will be dimensionless, there may be a better way to assign dimensionless units.
(The units returned by lk.flatten are actually incorrect.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Does the math of minus 1 e and then divided by 1 e make sense? What if somehow another input has flux in Jy? Then, what?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In the tag up today, Susan and I agreed to leave this as-is for now as not to change the currently behavior.

Copy link
Collaborator

@mustaric mustaric left a comment

Choose a reason for hiding this comment

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

I think these changes to make it work with lightkurve 2.0 look good. I can't comment on the test header part. I suggested one change to how to do the flatten -1 in that it starts as a quantity, so it should probably end as a quantity, but that quantity should be dimensionless. I doubt it will break anything if we don't make this change.

@mustaric
Copy link
Collaborator

mustaric commented Mar 1, 2021

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
Copy link
Collaborator Author

pllim commented Mar 1, 2021

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
Copy link
Collaborator

mustaric commented Mar 1, 2021

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
Copy link
Collaborator Author

pllim commented Mar 1, 2021

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
Copy link
Collaborator Author

pllim commented Mar 1, 2021

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

@mustaric
Copy link
Collaborator

mustaric commented Mar 4, 2021

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 pllim merged commit 07bca93 into spacetelescope:master Mar 4, 2021
@pllim pllim deleted the fix-ci branch March 4, 2021 15:21
@pllim
Copy link
Collaborator Author

pllim commented Mar 4, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants