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

Fix native_msg reader for ROI input and multi-part order file patterns #282

Merged
merged 5 commits into from May 7, 2018
Merged

Conversation

sjoro
Copy link
Collaborator

@sjoro sjoro commented May 4, 2018

Fixes issues #277 and #278. Fixes a bug in native_msg.py calibration, a wrong index was selected for the calibration coefficients. Fixes the area_extent-definition for native_msg. Adds two new areas to area.def. Performs code clean-up.

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.

Why weren't any of these changes for ROI inputs included before? Did no one ever test ROI before?

Additionally, we should probably migrate the areas file to be in YAML (since pyresample supports it now), but that doesn't have to happen in this PR.

Otherwise, I just have an issue with the file patterns.

file_patterns: ['{satid:4s}-{instr:4s}-MSG{product_level:2d}-0100-NA-{processing_time1:%Y%m%d%H%M%S.%f}000Z-{processing_time2:%Y%m%d%H%M%S}-{order_id:d}.nat',
'{satid:4s}-{instr:4s}-MSG{product_level:2d}-0100-NA-{processing_time1:%Y%m%d%H%M%S.%f}000Z-NA.nat',
'{satid:4s}-{instr:4s}-MSG{product_level:2d}-0100-NA-{processing_time1:%Y%m%d%H%M%S.%f}000Z-{order_id:d}.nat']
file_patterns: ['{satid:4s}-{instr:4s}-MSG{product_level:2d}-0100-NA-{processing_time1:%Y%m%d%H%M%S.%f}000Z-{order_id:s}.nat']
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't include the first original pattern which had a processing_time2. If this file pattern doesn't actually exist (check with person who originally added it) then this is fine.

Copy link
Collaborator Author

@sjoro sjoro May 4, 2018

Choose a reason for hiding this comment

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

processing_time2 is not used anywhere, the rest of the filename is now caught under order_id.

Copy link
Member

Choose a reason for hiding this comment

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

Ok so you expect the processing time to just be "eaten up" by the order id? Ok, I'd rather have two file patterns, but I'm ok with this if @adybbroe and @mraspaud are.

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, exactly. those file parts are not needed.

Copy link
Collaborator Author

@sjoro sjoro May 4, 2018

Choose a reason for hiding this comment

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

@djhoese what do mean with "Why weren't any of these changes for ROI inputs included before?" The area_extent?

@djhoese
Copy link
Member

djhoese commented May 4, 2018

And by "not included before" I mean in old satpy or mpop.

@djhoese djhoese changed the title Develop Fix native_msg reader for ROI input and multi-part order file patterns May 4, 2018
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.08%) to 64.392% when pulling 118f534 on sjoro:develop into 861c372 on pytroll:develop.

@codecov-io
Copy link

Codecov Report

Merging #282 into develop will decrease coverage by 0.07%.
The diff coverage is 6.02%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #282      +/-   ##
===========================================
- Coverage    64.47%   64.39%   -0.08%     
===========================================
  Files          114      114              
  Lines        13997    14008      +11     
===========================================
- Hits          9024     9020       -4     
- Misses        4973     4988      +15
Impacted Files Coverage Δ
satpy/readers/native_msg.py 18.46% <3.75%> (-1.22%) ⬇️
satpy/readers/native_msg_hdr.py 28.97% <66.66%> (+0.26%) ⬆️
satpy/readers/__init__.py 92.85% <0%> (-1.69%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 861c372...118f534. Read the comment docs.

@djhoese
Copy link
Member

djhoese commented May 4, 2018

@sjoro My question about ROI changes: This reader was part of mpop right? Or at least it existed in satpy before (version <0.9), right? Were people not using this reader with ROI datasets before this?

Basically, why were you the first one to find this bug? I figured it was some mistake you made in converting the reader to xarray/dask, but this PR looks like it is all new logic in the reader for figuring out of it is an ROI dataset.

@sjoro
Copy link
Collaborator Author

sjoro commented May 4, 2018

@djhoese ah, ok. yes, the reader was in mpop, but support for ROI-files was never implemented, only full disk files. we just got the ROI-area support fixed at the last workshop in Oslo. so, basically ROI-features were not tested before.

@djhoese
Copy link
Member

djhoese commented May 4, 2018

Ah ok, sounds good then.

@mraspaud mraspaud merged commit 7490ccf into pytroll:develop May 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants