Skip to content
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

Merged
merged 39 commits into from
May 7, 2024
Merged

Conversation

matteobachetti
Copy link
Member

@matteobachetti matteobachetti commented Apr 16, 2024

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 and GX) can now be loaded in Stingray. More versions of files can be added later

  • Calibration functions are moved to a separate mission_support module

  • Basic implementation

  • Working tests

  • Documentation on how to use this machinery

@pep8speaks
Copy link

pep8speaks commented Apr 16, 2024

Hello @matteobachetti! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 283:31: E203 whitespace before ':'
Line 284:34: E203 whitespace before ':'

Comment last updated at 2024-05-07 09:47:27 UTC

@matteobachetti matteobachetti marked this pull request as draft April 16, 2024 10:14
@matteobachetti matteobachetti marked this pull request as ready for review April 16, 2024 19:29
@matteobachetti matteobachetti marked this pull request as draft April 17, 2024 06:53
Copy link

codecov bot commented Apr 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.54%. Comparing base (ca74d3c) to head (800056b).

❗ Current head 800056b differs from pull request most recent head 2896fa8. Consider uploading reports for the commit 2896fa8 to get more accurate results

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.
📢 Have feedback on the report? Share it here.

@matteobachetti matteobachetti marked this pull request as ready for review April 17, 2024 21:01
@matteobachetti matteobachetti changed the title Support XTE Support calibration of XTE data in event mode Apr 18, 2024
Copy link
Collaborator

@mgullik mgullik left a 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
Copy link
Collaborator

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

Copy link
Member Author

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

Copy link
Collaborator

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 :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel the pain

@matteobachetti matteobachetti added this pull request to the merge queue May 7, 2024
Merged via the queue into main with commit 822f755 May 7, 2024
5 checks passed
@matteobachetti matteobachetti deleted the support_rxte branch May 7, 2024 10:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants