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

increase test coverage #39

Merged
merged 3 commits into from
Aug 25, 2023
Merged

Conversation

kecnry
Copy link
Member

@kecnry kecnry commented Aug 23, 2023

This PR increases overall test coverage from 85.65% to 92.95% (and simplifies some logic in the light curve parser).

NOTE: uncovered lines in events.py and template_mixin.py are scheduled to be moved upstream by #35 and #36, respectively, which should then increase percent coverage even further.

@codecov
Copy link

codecov bot commented Aug 23, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +7.30% 🎉

Comparison is base (874cd51) 85.65% compared to head (0d2cd06) 92.95%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #39      +/-   ##
==========================================
+ Coverage   85.65%   92.95%   +7.30%     
==========================================
  Files          24       24              
  Lines        1122     1093      -29     
==========================================
+ Hits          961     1016      +55     
+ Misses        161       77      -84     
Files Changed Coverage Δ
lcviz/helper.py 96.42% <ø> (+1.69%) ⬆️
lcviz/plugins/ephemeris/ephemeris.py 95.25% <ø> (+6.18%) ⬆️
lcviz/plugins/flatten/flatten.py 97.34% <ø> (+22.98%) ⬆️
lcviz/utils.py 90.72% <ø> (+3.34%) ⬆️
lcviz/parsers.py 97.29% <100.00%> (+18.22%) ⬆️
lcviz/state.py 100.00% <100.00%> (+40.47%) ⬆️

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kecnry kecnry force-pushed the increase-coverage branch from 601eaaf to 0d2cd06 Compare August 23, 2023 17:39
@kecnry kecnry marked this pull request as ready for review August 23, 2023 18:01
@kecnry kecnry requested a review from bmorris3 August 23, 2023 18:04
Comment on lines -20 to +19
# detect the type light curve in a FITS file:
with fits.open(file_obj) as hdulist:
filetype = detect_filetype(hdulist)

# get the constructor for this type of light curve:
filetype_to_cls = {
'KeplerLightCurve': KeplerLightCurve,
'TessLightCurve': TessLightCurve
}
cls = filetype_to_cls[filetype]
# read the light curve:
light_curve = cls.read(file_obj)
light_curve = lightkurve.read(file_obj)
Copy link
Member Author

Choose a reason for hiding this comment

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

@bmorris3 - any reason why we can't rely on lightkurve functionality here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm pretty sure this was a workaround in #10 until lightkurve/lightkurve#1299 was merged (see #10 (comment)). If this is simplification is working correctly now, then we're all good!

@@ -31,7 +31,7 @@ def _get_range_subset_bounds(self, subset_state, *args, **kwargs):
# TODO: use display units once implemented in Glue for ScatterViewer
# units = u.Unit(viewer.state.x_display_unit)
units = u.Unit(viewer.time_unit)
else:
else: # pragma: no cover
Copy link
Contributor

Choose a reason for hiding this comment

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

Well, that's one way to improve coverage 😉

Copy link
Member Author

Choose a reason for hiding this comment

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

it's my favorite way! (I just figured we didn't need coverage on simple input-errors, so might as well have that officially noted)

Copy link
Contributor

@rosteen rosteen left a comment

Choose a reason for hiding this comment

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

LGTM, tests pass, nothing objectionable in the code. Nice to simplify the parser.

@kecnry kecnry merged commit fbdc3e2 into spacetelescope:main Aug 25, 2023
@kecnry kecnry deleted the increase-coverage branch August 25, 2023 17:11
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.

3 participants