Fix property setting from astropy #814
Conversation
matteobachetti
force-pushed
the
fix_property_setting_from_astropy
branch
from
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.
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.
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.
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.
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. |
matteobachetti
force-pushed
the
fix_property_setting_from_astropy
branch
from
47744d1
to
1ed70bb
Compare
Resolve #813