-
Notifications
You must be signed in to change notification settings - Fork 141
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
Fix property setting from astropy #814
Conversation
59cf75c
to
1ab206c
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #814 +/- ##
=======================================
Coverage 96.43% 96.43%
=======================================
Files 45 45
Lines 9027 9034 +7
=======================================
+ Hits 8705 8712 +7
Misses 322 322 ☔ View full report in Codecov by Sentry. |
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.
Hi @matteobachetti,
as you said this is an easy fix.
Just a curiosity, why using fits as key returned an error?
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.
As @mgullik says, pretty straightforward. I'll approve it, but leave the comment on making the new warning more user-friendly there.
stingray/base.py
Outdated
and getattr(cls.__class__, key.lower(), None).fset is None | ||
): | ||
warnings.warn( | ||
f"{key} is a property of StingrayTimeseries without a setter. Not setting it" |
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.
Is this a warning that will be exposed to the user? If so, that seems pretty obscure (I don't think I would understand what to do with it, and how to fix it if necessary). Can we make this more user-friendly? What do you expect the user to do with the warning? Maybe another statement like "this is likely because ...", or "this means you can't do ..." or "you can safely ignore this warning" or something would maybe be helpful here.
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 it, hopefully it is more informative now
Our methods to read event lists from FITS files make choices about what information to read in the file. This is done by specifying "ogip" as the format. |
47744d1
to
1ed70bb
Compare
Resolve #813