-
Notifications
You must be signed in to change notification settings - Fork 172
Allow empty lightcurves #745
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
Conversation
Codecov Report
@@ Coverage Diff @@
## main #745 +/- ##
==========================================
- Coverage 97.13% 89.72% -7.42%
==========================================
Files 42 42
Lines 7919 7929 +10
==========================================
- Hits 7692 7114 -578
- Misses 227 815 +588
... and 14 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
7a84c8e to
3577e72
Compare
dhuppenkothen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems fine to me in principle, but should be reflected in the documentation/tutorial for Lightcurve before merging.
stingray/lightcurve.py
Outdated
|
|
||
| if time.size <= 1: | ||
| raise StingrayError("A single or no data points can not create " "a lightcurve!") | ||
| raise StingrayError("time and counts array are not of the same length!") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this might need an additional message for the failure mode where the user puts in a time array but not a counts array. I could imagine this happening if someone thinks they can generate a light curve from time-tagged arrivals by invoking Lightcurve(time=toas) rather than using the Lightcurve.make_lightcurve() class method. I would extend the error message here to include that for constructing a Lightcurve() object, but time and counts are required, and that for generating a Light curve from TOAs, use the class method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I changed a little bit the machinery, so that the existing error on the invalid counts array is more informative, and also includes the case where counts is None.
6469225 to
9b55bc3
Compare
9b55bc3 to
f81e0d0
Compare
dhuppenkothen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Resolve #744
Would something like this work for us? After all, I think it might make sense to have empty light curves, exactly like it makes sense, sometimes, to have empty event lists.