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

Invert bit position in quality flags #91

Merged
merged 6 commits into from
Dec 3, 2020

Conversation

carloshorn
Copy link
Collaborator

This PR corrects the quality flags which leads to differences in the masked scanlines as presented in #90.

@mraspaud
Copy link
Member

mraspaud commented Dec 2, 2020

Ouch. I think this means this code isn't tested properly?

@carloshorn
Copy link
Collaborator Author

Yes, I will add a more realistic test that really fills 32 bit big endian integers as expected from the files and get the corrupt mask for them.

@carloshorn
Copy link
Collaborator Author

Just noticed another thing:

pygac/pygac/klm_reader.py

Lines 755 to 760 in 817a82e

qual_flags[:, 4] = (
(self.scans["quality_indicator_bit_field"] << 24) >> 30)
qual_flags[:, 5] = (
(self.scans["quality_indicator_bit_field"] << 26) >> 30)
qual_flags[:, 6] = (
(self.scans["quality_indicator_bit_field"] << 28) >> 30)

This can be fixed by redefining the alias:
CH_3B_RS = 2**24
CH_3_CONTAMINATION = 2**24 # POD compatible alias
CH_3B_RS_ANOMALY = 2**25
CH_4_RS = 2**26
CH_4_CONTAMINATION = 2**26 # POD compatible alias
CH_4_RS_ANOMALY = 2**27
CH_5_RS = 2**28
CH_5_CONTAMINATION = 2**28 # POD compatible alias
CH_5_RS_ANOMALY = 2**29

like this
CH_3_CONTAMINATION = CH_3B_RS | CH_3B_RS_ANOMALY

Copy link
Member

@sfinkens sfinkens left a comment

Choose a reason for hiding this comment

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

Nice catch! Just two minor comments.

self.assertEqual(bits[0][-1], 1) # the last bit is filled
# The fatal flag fills only the first bit
self.assertEqual(bits[1].sum(), 1) # only one bit is filled
self.assertEqual(bits[1][0], 1) # only one bit is filled
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if this is required, but elements 3 and 4 of the quality_indicators array are not tested for byte order here

Copy link
Member

Choose a reason for hiding this comment

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

Typo in comment: I guess it should be "only the first bit is filled"

Copy link
Collaborator Author

@carloshorn carloshorn Dec 2, 2020

Choose a reason for hiding this comment

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

Not sure if this is required, but elements 3 and 4 of the quality_indicators array are not tested for byte order here

My idea is to check only the bit representation for the trivial cases (fist and last bit set). This should be sufficient to be sure that the bits look as expected for this test.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, fine!

@mraspaud mraspaud changed the title invert bit position in quality flags Invert bit position in quality flags Dec 3, 2020
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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants