-
Notifications
You must be signed in to change notification settings - Fork 144
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
Add calibration with RMF file #804
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #804 +/- ##
==========================================
+ Coverage 96.41% 96.42% +0.01%
==========================================
Files 45 45
Lines 8989 9016 +27
==========================================
+ Hits 8667 8694 +27
Misses 322 322 ☔ View full report in Codecov by Sentry. |
39d84cd
to
5220913
Compare
stingray/io.py
Outdated
the upper energy bound of each PI channel | ||
""" | ||
|
||
lchdulist = pf.open(rmf_file, checksum=True) |
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.
Probably not a big deal for a function this simple, but would it not be a bit more robust to use "with pf.open... as lchdulist" and then not needing to worry about closing the file by hand?
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.
Otherwise the response stuff lgtm
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.
@matteolucchini1 good point. I often do the explicit open/close because Astropy has a nonstandard behavior when associating the with
statement with the default memory mapping of FITS files (see discussion here: astropy/astropy#7404). I modified as requested, but forcing memmap=False, to avoid that bug
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,
Another useful addition to Stingray!
I was wondering if there is an option to load the rmf when Stingray load an event file. A rms_file variable set to None as default.
Another thing, is it worth adding a warning message when we load an event file, saying how the energies are calculated?
I guess that if rmf is not specified the conversion is done using default_nustar_rmf() right?
And why is file is named default_nustar_rmf()? I think it should be general, right?
p.s. one curiosity, which rmf have you chosen to add in the tests?
@mgullik good catch on the default_nustar_rmf, it was a leftover from HENDRICS' documentation with no real effect. |
The test rmf file is just taken from an old NuSTAR observation, I don't remember the details 😅 |
533019d
to
b3e61ad
Compare
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.
Thanks @matteobachetti, I can approve this PR.
I only have one last question, if the rmf is not specified what is the default method that Stingray uses to convert PI into Energies? If I'm not wrong the event file doesn't specified the energy of the single event. Wouldn't be good practice to specify, with a message when the event is read, which conversion Stingray uses?
bfb0b18
to
7534b16
Compare
No description provided.