-
Notifications
You must be signed in to change notification settings - Fork 12
Fix #34 to Stop Reading Multiple Event Data ExtensionsRead events stop #35
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
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #35 +/- ##
==========================================
+ Coverage 95.24% 95.92% +0.67%
==========================================
Files 3 4 +1
Lines 505 540 +35
==========================================
+ Hits 481 518 +37
+ Misses 24 22 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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 @JawadAhmadCS ! Thanks for addressing this issue! Could you also add some tests that try to capture what these checks are doing? E.g., ensure that if a table has two "TIME" columns in two different HDUs, only the first is read?
colnames = FITSIO.colnames(hdu) | ||
|
||
if "TIME" in colnames | ||
# If both TIME and ENERGY columns are found and we haven't read the event data yet |
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.
My suggestion here would be to simplify the logic:
if isempty(times) && ("TIME" in colnames)
times = convert(Vector{T}, read(hdu, "TIME"))
end
if isempty(energies) && ("ENERGY" in colnames)
energies = convert(Vector{T}, read(hdu, "ENERGY"))
end
This will then read the first "TIME" and "ENERGY" column. We can do more sophisticated things here too, but I think this should be sufficient for our first goals.
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.
You could add a third check for when the data is read that we break out of the for loop, but since this is currently also being used to read in the headers, that would require more refactoring. It's fine for this PR just to fix one issue in a good-enough way, and we can iterate later. We also will want to be a bit more careful with how we read in FITS headers, but that's low priority at the moment.
Thanks for your great review @fjebaker but before starting proper work on this, I just want to confirm from @kashish2210 whether he plans to work on this issue. Since he worked on it before, I want to avoid duplication of effort. Kashish, please let me know |
This PR fixes the issue in #34, where event data (TIME and ENERGY) was being read from multiple HDUs. The code now stops reading event data once it’s found, preventing unwanted overwriting from other extensions.