Fixes STIX file loading and lightcurve/spectrogram plotting #98
Conversation
|
Hi @natsat0919. Thanks so much for contributing this PR. Your changes appear to be breaking a lot of the tests. Can you clarify why this is? Is it possible you've made changes in certain places that a valid for STIX but not other instruments? |
|
Hi @DanRyanIrish, I'm missing a changelog file that I'll add. I'm still trying to figure out why the other tests are failing, but I think that it's because of changing the fundamental loading function for STIX files and the tests aren't adjusted to that? The changes that I have done were only for STIX (mainly in the stix_spec_code.py), so it shouldn't affect any other instruments |
|
Following up on our discussion @natsat0919 and @KriSun95: I think we should be careful and explicit as to which kind of "spectrogram" we load in, i.e. the one produced by IDL or the one from the data center, and pay particular attention to the statistical errors that are being used. I would be happy to review some of the code in this PR and see how we can make sure that distinction is clear. Or we can merge the PR and do that later. Thoughts? |
|
@settwi I agree, we definitely need to get that sorted as a priority. I think right now is the best to get this out as the basic working version (just updating what we already have) and build up from there? "Luckily", this code only works for the data centre spectrograms, so it will be obvious if the IDL spectrograms are loaded in. Also the STIX srm and spectral files tests are outdated (hence I got so many fails) so I'm sorting that out as well. Meanwhile, we can think what's the best way to implement the "distinction" when loading in? |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #98 +/- ##
==========================================
+ Coverage 55.01% 55.06% +0.04%
==========================================
Files 19 19
Lines 3119 3122 +3
==========================================
+ Hits 1716 1719 +3
Misses 1403 1403
☔ View full report in Codecov by Sentry. |
|
This looks great, thanks @natsat0919 ! I've had a play about with the changes and it works fine for me. Playing about with it has pointed out a small but important bug elsewhere (that I need to fixed) but I can't see anything wrong with this PR. I definitely agree with both you and @settwi about moving on to think about how to separate loading in of the two STIX file formats for the spectra (data centre and IDL). I'll return to this later today, add a review for the PR, and then move on to approve the merge if there are no other objections. |
|
Do we consider this PR ready to merge? |
|
@DanRyanIrish yes, I just did some very minor refactoring changes after Kris's approval. It should be ready to merge |
* Fix STIX file loading to be compatible with the final format of STIX files * Fix STIX lightcurve and spectrogram plotting
This PR fixes the functions for handling STIX spectral and SRM files, this means that the Sunxspex class will be apple to correctly load in the new/final version of STIX files. I'm mainly fixing formatting of keywords and data, change of time units, format of energy bins and counts that are given in the spectral files. I have also fixed the incorrect plotting of background and event time-ranges for STIX lightcurves and spectrograms (examples in issue #97).