-
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
Support calibration of XTE data in event mode #816
Conversation
Hello @matteobachetti! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2024-05-07 09:47:27 UTC |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #816 +/- ##
==========================================
+ Coverage 96.50% 96.54% +0.03%
==========================================
Files 45 48 +3
Lines 9149 9254 +105
==========================================
+ Hits 8829 8934 +105
Misses 320 320 ☔ View full report in Codecov by Sentry. |
6a73d70
to
8720ce6
Compare
b5105da
to
34a2e0c
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.
Hi @matteobachetti,
this is super! Having the possibility to work on the RXTE data is a big improvement for Stingray.
Sorry if I write something that is already implemented in other places of the code.
my questions/suggestions:
- in one of the doc string you wrote that the conversion from PHA to absolute PHA is done by the function pca_calibration_func using TEVTB2 keyword. However, I don't see how you use the TEVTB2 keyword in that function. Also the doc string of pca_calibration_func says that it's "A function that converts PHA channel to energy for PCU 0"
- does it make sense to create a test that checks if the conversion from channel to energy is correct? I've see a test on the data.pi_list, but I think this is for channels.
- Adding a test using a mission that is either not supported by Stingray or doesn't exist and catching the error message.
Cheers
50 46 13.48 15.93 18.46 21.65 22.09 21.04 | ||
51 47 13.74 16.25 18.83 22.08 22.54 21.46 | ||
52 48 14.01 16.57 19.20 22.52 22.98 21.88 | ||
53 49 14.28 16.89 19.56 22.95 23.43 22.30 |
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.
The format of the table changes here, it can get confusion to read and comparte
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 is the original table from the RXTE website, I maintained their format. I preferred to make the original table available instead of storing in a different format. Probably an excess of caution, but I figured it would have been easier to track possible changes over time
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 remember that format from years ago...I understand your point, even though looking at the table formatted in that way make me think that many mistakes had been and will be made :)
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 feel the pain
I had to change a little the structure, so that mission support is a separate beast than simple I/O.
XTE/PCA is now supported with the recommended calibration. Science Event files from the XTE archive (those starting with
SE
andGX
) can now be loaded in Stingray. More versions of files can be added laterCalibration functions are moved to a separate
mission_support
moduleBasic implementation
Working tests
Documentation on how to use this machinery