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

Calibration mode can now be passed via a keyword argument #543

Merged
merged 8 commits into from Mar 13, 2019

Conversation

ColinDuff
Copy link
Contributor

This PR includes the code need to pass the Calibration mode , needed for MSG Calibration, using keyword arguments

This would close #521 if approved

flake8 passed for all 3 files

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.

Looks good. I have few code cleanup things and other suggestions that I'd like to discuss. See comments below.

satpy/readers/native_msg.py Outdated Show resolved Hide resolved
satpy/readers/yaml_reader.py Outdated Show resolved Hide resolved
satpy/readers/yaml_reader.py Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Dec 10, 2018

Coverage Status

Coverage increased (+0.1%) to 78.223% when pulling 6747065 on ColinDuff:gsics_fix into f3358ca on pytroll:master.

@codecov
Copy link

codecov bot commented Dec 10, 2018

Codecov Report

Merging #543 into master will decrease coverage by <.01%.
The diff coverage is 76.92%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #543      +/-   ##
==========================================
- Coverage   78.07%   78.07%   -0.01%     
==========================================
  Files         137      137              
  Lines       19890    19894       +4     
==========================================
+ Hits        15530    15533       +3     
- Misses       4360     4361       +1
Impacted Files Coverage Δ
satpy/readers/__init__.py 95.12% <100%> (ø) ⬆️
satpy/readers/yaml_reader.py 92.6% <100%> (+0.03%) ⬆️
satpy/readers/seviri_l1b_native.py 51.32% <50%> (-0.02%) ⬇️

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 f3358ca...6747065. Read the comment docs.

@mraspaud mraspaud added enhancement code enhancements, features, improvements component:readers labels Dec 11, 2018
@mraspaud mraspaud added this to In progress in Pytroll Contribution Week at Eumetsat via automation Dec 11, 2018
Pytroll Contribution Week at Eumetsat automation moved this from In progress to Needs review Dec 11, 2018
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.

Good job! A couple of things missing:

  • Documentation about the calibration mode switch (could be done in the docstring of the native_msg module)
  • Tests

Also, would this work with hrit data too ?

@ColinDuff
Copy link
Contributor Author

using of the calibration mode kwarg was added to the native_msg_reader class.
A test for the gsics calibration mode can be added later if that is ok
I dont use hrit_msg format so i cant easily test any changes - we will do this later in a separate pr

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. Just a simple test and we're good to merge imo.

satpy/readers/native_msg.py Outdated Show resolved Hide resolved
@ColinDuff
Copy link
Contributor Author

Re creating a test: In theory should we not instantiate a Scene object for native_msg reader and check the calibration mode but the init method tries to open/read in an actual file to read the header and trailer. I am not sure how to create a test for this scenario.

@mraspaud
Copy link
Member

You could do one test for instantiating the file handler with calib_mode=gsics and make sure the right part of the metadata is accessed, and another one that instantiates a scene object with a fake filehandler, to make sure the fh_kwargs are passed to the filehandler's __init__.

@ColinDuff
Copy link
Contributor Author

Re creating a test. I will wait until the test reader name changes are in master if thats ok

@djhoese
Copy link
Member

djhoese commented Mar 1, 2019

This PR needs a merge with master to fix the conflicts.

sfinkens added a commit to sfinkens/satpy that referenced this pull request Mar 4, 2019
@sfinkens
Copy link
Member

sfinkens commented Mar 4, 2019

@ColinDuff I would like to propose adding a line VIS_CHANNELS = ['HRV', 'VIS006', 'VIS008', 'IR_016'] to satpy.readers.seviri_base.py which can then be used by both the native and the HRIT reader (see https://github.com/pytroll/satpy/pull/636/files#diff-6638815b577a979fa2e187c166216a28).

@mraspaud
Copy link
Member

@ColinDuff Where are we with this PR ? It would be nice to see the conflicts fixed and the tests implemented so that we can merge it.

@ColinDuff
Copy link
Contributor Author

apologies but i have to concentrate on a new MPEF release for now. When i have time i can resolve the issues and have the PR in a state to be merged. will try to do so soon

@ColinDuff
Copy link
Contributor Author

just pushed to get into sync with satpy master, new filenames etc. will add a test soon


def tearDown(self):
pass

Copy link
Contributor

Choose a reason for hiding this comment

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

W293 blank line contains whitespace

Pytroll Contribution Week at Eumetsat automation moved this from Needs review to Reviewer approved Mar 12, 2019
Pytroll Contributor's Week - Spring 2019 - Madison, WI automation moved this from In progress to Reviewer approved Mar 12, 2019
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.

Waiting on #647 to fix the appveyor failures, then I'll merge this.

@mraspaud mraspaud merged commit caa3653 into pytroll:master Mar 13, 2019
Pytroll Contribution Week at Eumetsat automation moved this from Reviewer approved to Done Mar 13, 2019
Pytroll Contributor's Week - Spring 2019 - Madison, WI automation moved this from Reviewer approved to Done Mar 13, 2019
@ColinDuff ColinDuff deleted the gsics_fix branch November 11, 2020 08:17
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
Development

Successfully merging this pull request may close these issues.

Interactively set the Calibration Mode when creating the Scene Object
6 participants