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

Multiple functionalities added to stx_science_data_lightcurve #132

Merged
merged 3 commits into from
Mar 13, 2023
Merged

Multiple functionalities added to stx_science_data_lightcurve #132

merged 3 commits into from
Mar 13, 2023

Conversation

afbattaglia
Copy link
Collaborator

Hi @grazwegian

I've added two functionalities to stx_science_data_lightcurve (see issue #113):

  1. FITS files concatenation: the user can now pass multiple FITS files under fits_path, as array of string, and the output will be the concatenation of all FITS files given as an input. This is particularly useful if we want to avoid digital steps in the time profiles that we can see in the spectrogram data. Since the cpd are requested in multiple FITS files, now we can get a single time profile out of multiple cpd FITS files.

  2. Sub-interval extraction: this goes in the opposite direction as point 1. The keyword time_range has been added, so that we can get a lightcurve for a sub-interval only. This is useful if we just need a relatively short time profile out of a long spectrogram FITS file.

…iles concatenation and sub-interval extraction
@samaloney
Copy link
Collaborator

This looks great one question I have is how would this work if the pixel/detector/energy masks change between the the supplied fits files, maybe this is handled in the ospex part of the code?

@afbattaglia
Copy link
Collaborator Author

Yes, this is handled in the OSPEX part of the code. What we do here is to extract the fluxes (in cnts/s/keV/cm^2) from the OSPEX object separately for each FITS file and then concatenate these flux arrays (as well as times and other stuff). Maybe not the best solution from the computational cost and efficiency point of view, but it seems to work fine.
This is what we discussed with @grazwegian a few months ago. If there is another (better) solution I can try to implement it in the near future

@samaloney samaloney self-requested a review February 9, 2023 11:05
I've applied a few small corrections taking into account Shane's comments:
 - doc string of ´fits_path´ updated
 - doc string of ´time_range´ updated
@grazwegian
Copy link
Collaborator

Hi. In testing I found an issue that it wasn't removing many overlapping time bins - I think this is due to the way it determines the exact start time from the FITS header and the fact these might not exactly align so when keeplist = where(this_diff ne 0) is called in line 217 there are many time intervals where the difference is very small but not 0 and these are not removed.

this_diff

@afbattaglia
Copy link
Collaborator Author

Great, thanks a lot for spotting this Ewan! Ok, then if I have understood well your plot (and issue) we could replace keeplist = where(this_diff ne 0) of line 217 with keeplist = where(abs(this_diff) ge 0.5), since in any case we do not expect to have bins less than 0.5 s cadence and the short time bins have been already taken into account earlier in the code.

Would this change be a solution of the issue? Or do we need to be a little conservative and set it a bit lower, such as keeplist = where(abs(this_diff) ge 0.48)?

@grazwegian
Copy link
Collaborator

Yes I think that would solve the issue. I would think we should err on the side of being a bit conservative in case of numerical issues. I am not sure if at some point we might try a minimum time of 0.4 or 0.3 but we can easily update in future if that becomes an issue.

Thanks to Ewan's review, line 217 has been modified to solve the issue when removing overlapping time bins.
@samaloney samaloney merged commit 5b9f32d into i4Ds:master Mar 13, 2023
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.

3 participants