-
-
Notifications
You must be signed in to change notification settings - Fork 583
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
Fixed XRSTimeSeries quality flags from breaking parsing of some XRS data. #6410
Conversation
@@ -235,8 +235,12 @@ def _parse_netcdf(filepath): | |||
flux_name_b = flux_name_a.replace("a", "b") | |||
xrsa = np.array(h5nc[flux_name_a]) | |||
xrsb = np.array(h5nc[flux_name_b]) | |||
xrsa_quality = np.array(h5nc[flux_name_a.replace("flux", "flags")]) | |||
xrsb_quality = np.array(h5nc[flux_name_b.replace("flux", "flags")]) | |||
flux_flag_a = h5nc.variables.get("a_flags") or h5nc.variables.get("xrsa_flags") or h5nc.variables.get("xrsa_flag") |
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.
That is quite annoying that they vary at least 3 ways.
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 know the stuff has been running for 60 years or something so changes are excused but there is so much stuff like this with GOES which make it a pain.
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 think we should perhaps think about this some more and maybe have specific 1m/ 2s/background timeseries classes or something
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.
Even within the same data series they are inconsistent with file names/column names/metadata so while it might be an idea, it doesn't make this kind of thing any tidier.
As an aside I have a scraper for 1m data (plus reader for pre 2009 1m data) but its ugly (secondary client rather than using attrs to select series) so haven't tried to put it in sunpy yet.
Co-authored-by: Nabil Freij <nabil.freij@gmail.com>
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.
Looks 👍 - I don't suppose there are some small files we can add to our test suite to make sure this doesn't break in the future?
So I made a test file from a 1m avg goes file and added it to the test suite. I am happy with the PR now as long as you lot are. |
Owee, I'm MrMeeseeks, Look at me. There seem to be a conflict, please backport manually. Here are approximate instructions:
And apply the correct labels and milestones. Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon! Remember to remove the If these instructions are inaccurate, feel free to suggest an improvement. |
PR Description
Closes #6409
Currently #6260 checks for 2 possible netcdf names for the XRS quality flag column 'xrs[a/b]_flags' and '[a/b]_flag' , this stops TimeSeries from loading in some XRS data files due to KeyError when neither of those exist.
This PR adds support for a 3rd flag column name: 'xrs[a/b]_flag' but maybe there is a 4th (or more) out there though