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.

Sign up for GitHub

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

Jump to bottom

Quality indicators #72

Merged
merged 8 commits into from
Jul 20, 2020
Merged

Quality indicators #72

merged 8 commits into from
Jul 20, 2020

Conversation

carloshorn
Copy link
Collaborator

This PR should close #60.

It implements the quality indicator mapping as enum.IntFlag for KLM and POD readers and moves the methods reader._get_corrupt_mask and get_qual_flags to the parent class.

@carloshorn
Copy link
Collaborator Author

It appears to me like there is a test missing where the mask property is built from given quality indicators. Currently all tests replace the call to _get_corrupt_mask with a mock otherwise, there would have been a test failing before the last commit. Should I write a new test, or include it into the already existing test TestGacReader.test_get_corrupt_mask

mraspaud
mraspaud reviewed Jul 16, 2020
View reviewed changes
Copy link
Member

@mraspaud mraspaud left a comment

Choose a reason for hiding this comment

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

Looks good overall, I really like how using an enum makes things much clearer. I just have a few questions inline.

pygac/klm_reader.py
Comment on lines +36 to +40
try:
from enum import IntFlag
except ImportError:
# python version < 3.6, use a simple object without nice representation
IntFlag = object
Copy link
Member

Choose a reason for hiding this comment

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

Can we decide that pygac is python >= 3.6 only? It would make a lot simpler

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Absolutely, I would be one of the first to vote for it.
There are already a lot of special python 2 hacks and test cases that we skip, so it would totally make sense.
I don't know all PyGAC users, but nowadays it is easy to set up a python >= 3.6 environment using anaconda, so I guess no one will complain.

Copy link
Member

Choose a reason for hiding this comment

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

@sfinkens @abhaydd any opinion on this?

Copy link
Member

Choose a reason for hiding this comment

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

I think that's fine. But let's do this in a separate PR ;)

pygac/klm_reader.py Outdated Show resolved Hide resolved
pygac/klm_reader.py Outdated
Comment on lines 84 to 93
CH_3B_RS = 2**24
CH_3B_RS_ANOMALY = 2**25
CH_4_RS = 2**26
CH_4_RS_ANOMALY = 2**27
CH_5_RS = 2**28
CH_5_RS_ANOMALY = 2**29
# using same name as for POD
CH_3_CONTAMINATION = 2**24 # Channel 3 solar blackbody contamination
CH_4_CONTAMINATION = 2**26 # Channel 4 solar blackbody contamination
CH_5_CONTAMINATION = 2**28 # Channel 5 solar blackbody contamination
Copy link
Member

Choose a reason for hiding this comment

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

Something is fishy here, 224, 226 and 2**28 are used twice. Does this work?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, it is possible to define several aliases, which I did to have the same names as in the POD QFlags. In this case both flags have the same meaning so it should not be very confusing. A more "critical" flag is

    SYNC_INVALID = 2**10  # Frame sync word not valid
    FLYWHEELING = 2**10  # Flywheeling detected during this frame

see the docstring note.
I could implement the correct Flag in the constructor after checking the date, but the static implementation is simpler and I hope that someone using this flag knows which definition is applicable...
Any suggestion?

Copy link
Member

Choose a reason for hiding this comment

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

Works for me. Just to make it clearer, maybe having the pod name close the klm would help?

Suggested change
CH_3B_RS = 2**24
CH_3B_RS_ANOMALY = 2**25
CH_4_RS = 2**26
CH_4_RS_ANOMALY = 2**27
CH_5_RS = 2**28
CH_5_RS_ANOMALY = 2**29
# using same name as for POD
CH_3_CONTAMINATION = 2**24 # Channel 3 solar blackbody contamination
CH_4_CONTAMINATION = 2**26 # Channel 4 solar blackbody contamination
CH_5_CONTAMINATION = 2**28 # Channel 5 solar blackbody contamination
CH_3B_RS = 2**24
# POD compatible name, Channel 3 solar blackbody contamination
CH_3_CONTAMINATION = 2**24
CH_3B_RS_ANOMALY = 2**25
CH_4_RS = 2**26
# POD compatible name, Channel 4 solar blackbody contamination
CH_4_CONTAMINATION = 2**26
CH_4_RS_ANOMALY = 2**27
CH_5_RS = 2**28
# POD compatible name, Channel 5 solar blackbody contamination
CH_5_CONTAMINATION = 2**28
CH_5_RS_ANOMALY = 2**29

pygac/klm_reader.py
SYNC_INVALID = 2**10 # Frame sync word not valid
FLYWHEELING = 2**10 # Flywheeling detected during this frame
BIT_SLIPPAGE = 2**11 # Bit slippage detected during this frame
TIP_PARITY = 2**23 # TIP parity error detected
Copy link
Member

Choose a reason for hiding this comment

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

Out of curiosity, why are 12-22 not included ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Simply because they are not defined for KLMs

bit 20: bit slippage detected during this frame
bits 19-9: <zero fill>
bit 8: TIP parity error detected

Copy link
Member

Choose a reason for hiding this comment

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

ok, maybe a comment here would be nice then, just to clarify.

sfinkens
sfinkens approved these changes Jul 17, 2020
View reviewed changes
Copy link
Member

@sfinkens sfinkens left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Very nice, that improves readability a lot!

Edit: I think a separate test for _get_corrupt_mask would be nice.

pygac/klm_reader.py
Comment on lines +54 to +61
Source:
KLM guide
Table 8.3.1.3.3.1-1. Format of packed LAC/HRPT Data Sets (Version 2, pre-April 28, 2005).
Table 8.3.1.3.3.2-1. Format of LAC/HRPT Data Record for NOAA-N (Version 5, post-November 14,
2006, all spacecraft).
Table 8.3.1.4.3.1-1. Format of packed GAC Data Record for NOAA KLM (Version 2, pre-April 28, 2005).
Table 8.3.1.4.3.2-1. Format of GAC Data Record for NOAA-N (Version 4, post-January 25, 2006,
all spacecraft).
Copy link
Member

Choose a reason for hiding this comment

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

Nice!

mraspaud
mraspaud approved these changes Jul 20, 2020
View reviewed changes
Copy link
Member

@mraspaud mraspaud left a comment

Choose a reason for hiding this comment

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

LGTM

@mraspaud
Copy link
Member

@carloshorn Is this ready for merge or do you want to implement @sfinkens 's suggestion?

@carloshorn
Copy link
Collaborator Author

The suggested test for _get_corrupt_mask is already implemented (see last commit)., so it is ready from my side.

@sfinkens
Copy link
Member

LGTM!

@mraspaud
Copy link
Member

Ah, yes, sorry, the name of the tests confused me into thinking it was testing something else. Merging.

@mraspaud mraspaud merged commit 64d7fd3 into pytroll:master Jul 20, 2020
@sfinkens sfinkens mentioned this pull request Nov 26, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sfinkens sfinkens sfinkens approved these changes

@mraspaud mraspaud mraspaud approved these changes

Assignees

@carloshorn carloshorn

Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Improve readability of quality indicators bit unpacking
4 participants
@carloshorn @mraspaud @sfinkens @stickler-ci