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

Improve MiRS reader handling of missing metadata #1671

Merged
merged 38 commits into from May 19, 2021

Conversation

joleenf
Copy link
Contributor

@joleenf joleenf commented May 13, 2021

  • adds metadata to the yaml file for files which do not have internal metadata
  • combines metadata that may be in file to yaml attributes if they are different but file takes precedence. This is meant to address the loss of the units from the final metadata in the previous version
  • applies valid_range, fill_data and scale before limb correction
  • all mirs data files will get more information about lat/lon coordinates from the yaml
  • adjusts "Kelvin" to "K" for consistency within the project
  • adds tests for units, and fill_value
  • add coordinates to fake test data set to match how actual files look to open_dataset.

Remove platform name from parameterize in last test for kwargs because
they are not used in that test.  Add a test for the NOAA-20 platform
when the limb_correction assertions are tested and platform name is
tested.
Merge remote-tracking branch 'upstream/master'
Fixes a bug in which valid range was ignored.
Change get_valid_range=>apply_valid_range and check for case of valid_range not None
only read fake_data one time, not every time check_valid_range is called.
…including _FillValue scale.

- Fill value needs to be scaled in these files.
- Try to combine yaml and file attributes with file attributes taking precedence over yaml.
- Make sure units is saved in data array attributes
- Need to make sure that yaml metadata when present is used and supplements
  attributes from data_arr.
- Apply scaling to valid_range and fill_value
- move location of apply_attributes
- change name of get_metadata
- apply the attributes before the limb correction so that scaling, valid_range, fill are applied correctly
- the yaml had some copy and pasted errors that would make it useless and unreadable
- remove BT from yaml and handle attributes in reader
- use the yaml for lat/lon attributes from every file
…eded for mirs_gpm

Added because I did not consider that the limb correction is applied based on sensor which
is assigned by platform.  The mirs_gpm addition was not needed so I removed it.
@joleenf joleenf requested a review from mraspaud as a code owner May 13, 2021 14:24
@@ -433,6 +465,7 @@ def _available_btemp_datasets(self):
'name': new_name,
'description': desc_bt,
'units': 'K',
'scale_factor': self.nc['BT'].attrs['scale_factor'],
Copy link
Member

Choose a reason for hiding this comment

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

Why only scale_factor and not add_offset?

Comment on lines 53 to 54
scale_factor: 0.1
_FillValue: -99.9
Copy link
Member

Choose a reason for hiding this comment

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

Both scale_factor and _FillValue aren't provided in the files?

@djhoese djhoese changed the title Mirs metadata updates Improve MiRS handling of missing metadata May 13, 2021
@djhoese djhoese changed the title Improve MiRS handling of missing metadata Improve MiRS reader handling of missing metadata May 13, 2021
@djhoese djhoese self-assigned this May 13, 2021
@codecov
Copy link

codecov bot commented May 13, 2021

Codecov Report

Merging #1671 (1649241) into main (2a1110e) will decrease coverage by 0.33%.
The diff coverage is 96.96%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1671      +/-   ##
==========================================
- Coverage   92.66%   92.32%   -0.34%     
==========================================
  Files         258      258              
  Lines       37988    38123     +135     
==========================================
- Hits        35200    35196       -4     
- Misses       2788     2927     +139     
Flag Coverage Δ
behaviourtests 4.84% <0.00%> (+0.01%) ⬆️
unittests 92.86% <96.96%> (+0.06%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
satpy/readers/mirs.py 76.00% <96.15%> (+2.61%) ⬆️
satpy/tests/reader_tests/test_mirs.py 100.00% <100.00%> (ø)
satpy/utils.py 25.33% <0.00%> (-63.56%) ⬇️
satpy/dataset/dataid.py 94.05% <0.00%> (-0.23%) ⬇️
satpy/resample.py 80.24% <0.00%> (-0.11%) ⬇️
satpy/writers/cf_writer.py 95.17% <0.00%> (-0.02%) ⬇️
satpy/tests/test_dataset.py 100.00% <0.00%> (ø)
satpy/readers/file_handlers.py 97.18% <0.00%> (ø)
satpy/tests/test_file_handlers.py 100.00% <0.00%> (ø)
satpy/tests/reader_tests/test_satpy_cf_nc.py 100.00% <0.00%> (ø)
... and 9 more

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 2a1110e...1649241. Read the comment docs.

If the information is in the metadata of the file, use that, otherwise
define in the yaml.  Fix getting keys from ds_info in apply_attributes
function so it does not needlessly break data that has no units.
@joleenf joleenf marked this pull request as draft May 14, 2021 12:36
…cients is consistent

Old coefficients have a blank line at end, new coefficients do not.  Previously, n_chn and
n_fov were fixed in the coefficient reading.  So, return to that so it is easier to swap
new and old coefficients if necessary for testing.
@ghost
Copy link

ghost commented May 15, 2021

Congratulations 🎉. DeepCode analyzed your code in 2.272 seconds and we found no issues. Enjoy a moment of no bugs ☀️.

👉 View analysis in DeepCode’s Dashboard | Configure the bot

@joleenf joleenf marked this pull request as ready for review May 15, 2021 14:06
@joleenf joleenf closed this May 15, 2021
@joleenf joleenf reopened this May 15, 2021
@joleenf joleenf requested a review from djhoese May 18, 2021 20:28
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.

LGTM. Thanks

@djhoese djhoese merged commit 09ce355 into pytroll:main May 19, 2021
@djhoese
Copy link
Member

djhoese commented May 19, 2021

FYI, I squashed all your commits into a single commit and merged it. Looking at your fork, I think your main has commits that it shouldn't so your pull requests are getting out of sync with what github expects for going in the main branch. You may want to do some git magic to get your main branch to the proper commit.

@joleenf
Copy link
Contributor Author

joleenf commented May 19, 2021

What do you want me to do with my main branch?

@joleenf
Copy link
Contributor Author

joleenf commented May 19, 2021

I should start with what I think I understand. I have too many commits that are more intermediate/confused and are not needed. This is causing problems with the main branch of this repo when trying to merge my fork into pytroll/satpy main. How do I know that my main branch is at the proper commit? What do you mean by git magic?

@djhoese
Copy link
Member

djhoese commented May 19, 2021

Let's talk in the CSPP slack.

@joleenf joleenf deleted the mirs_metadata branch May 19, 2021 12:35
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

2 participants