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

Hardcoding of mersi2 l1b reader valid_range for channel 24 and 25 as these are wrong in the HDF data #874

Merged

Conversation

TAlonglong
Copy link
Collaborator

Add harcoding of mersi2 l1b reader valid_range for channel 24(10.8) and 25(12.0) as these have wrong values in the HDF data

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.

Thanks for the quick PR. I'm wondering if we should be proactive and check the version of the file format (if there is such info in the file ?) also, so that it'll be forward compatible too ?

@mraspaud mraspaud added component:readers enhancement code enhancements, features, improvements labels Aug 12, 2019
@mraspaud mraspaud added this to the v0.17.0 milestone Aug 12, 2019
@TAlonglong
Copy link
Collaborator Author

I can't see any version information. What the PR do is that it check the current known invalid value and corrects this.

When the format is updated the valid_range value is not altered.

Copy link
Member

@djhoese djhoese left a comment

Choose a reason for hiding this comment

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

If this wasn't something you needed right away, I'd say we should just wait. Since we don't know when the fix will be released I'm ok with what you have. You do have unnecessary parentheses and should have a space after the if. Does flake8 allow this?

@TAlonglong
Copy link
Collaborator Author

TAlonglong commented Aug 12, 2019

flake8 did not complain at least. But I removed the parentheses and fixed the space.

@djhoese
Copy link
Member

djhoese commented Aug 22, 2019

@mraspaud @TAlonglong What do we think on this? Turns out I need this and it doesn't look like a fix is coming soon (or at least there wasn't any kind of indication) as I had hoped.

FYI merged with master this should pass tests again.

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 mraspaud merged commit d90e7f5 into pytroll:master Aug 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:readers enhancement code enhancements, features, improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants