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.

Sign up for GitHub

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

Jump to bottom

Add support for NUCAPS Science EDRs #1061

Merged
merged 10 commits into from
Feb 20, 2020
Merged

Add support for NUCAPS Science EDRs #1061

merged 10 commits into from
Feb 20, 2020

Conversation

tommyjasmin
Copy link
Contributor

  • Closes #xxxx
  • Tests added and test suite added to parent suite
  • Tests passed
  • Passes flake8 satpy
  • Fully documented
  • Add your name to AUTHORS.md if not there already

@djhoese djhoese added component:readers enhancement code enhancements, features, improvements labels Feb 6, 2020
@djhoese djhoese self-assigned this Feb 6, 2020
@djhoese djhoese added this to the v0.20.0 milestone Feb 6, 2020
@coveralls
Copy link

coveralls commented Feb 6, 2020 •
edited
Loading

Coverage Status

Coverage increased (+0.02%) to 88.893% when pulling 1c6e20c on tommyjasmin:nucaps-sci-edr-support into f0e9116 on pytroll:master.

@codecov
Copy link

codecov bot commented Feb 6, 2020 •
edited
Loading

Codecov Report

Merging #1061 into master will increase coverage by 0.77%.
The diff coverage is 95.09%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1061      +/-   ##
==========================================
+ Coverage   88.11%   88.89%   +0.77%     
==========================================
  Files         191      192       +1     
  Lines       29568    29657      +89     
==========================================
+ Hits        26054    26363     +309     
+ Misses       3514     3294     -220
Impacted Files Coverage Δ
satpy/writers/__init__.py 86.02% <ø> (ø) ⬆️
satpy/resample.py 90.63% <ø> (-0.02%) ⬇️
satpy/readers/geocat.py 88.46% <ø> (+0.64%) ⬆️
satpy/tests/reader_tests/test_geocat.py 100% <100%> (+3.03%) ⬆️
satpy/readers/xmlformat.py 89.65% <100%> (+72.41%) ⬆️
satpy/tests/reader_tests/test_eps_l1b.py 100% <100%> (ø)
satpy/composites/__init__.py 79.03% <100%> (ø) ⬆️
satpy/tests/compositor_tests/__init__.py 99.61% <100%> (ø) ⬆️
satpy/readers/nucaps.py 89.04% <100%> (ø) ⬆️
satpy/tests/reader_tests/__init__.py 98.48% <100%> (+0.02%) ⬆️
... and 5 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 d1bb747...1c6e20c. Read the comment docs.

djhoese
djhoese requested changes Feb 7, 2020
View reviewed changes
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.

Nice job. For the science edr tests, could you make a separate test dataset that has the same dimension names as your files? Might require copying the FakeNetCDF4FileHandler2 object. Otherwise I had a couple other small comments. Thanks for doing this.

satpy/tests/reader_tests/test_nucaps.py Outdated
@@ -337,11 +335,195 @@ def test_load_pressure_levels_single_and_pressure_levels(self):
self.assertTupleEqual(pl_ds.shape, (1,))


# For testing Science EDRs
Copy link
Member

Choose a reason for hiding this comment

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

Redundant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wow, yes.

satpy/tests/reader_tests/test_nucaps.py Outdated
import unittest2 as unittest
else:
import unittest
import unittest

try:
from unittest import mock
Copy link
Member

Choose a reason for hiding this comment

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

If you have to make other changes you could remove this try/except and only do from unittest import mock.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, yeah you mentioned that.

@tommyjasmin
Copy link
Contributor Author

Nice job. For the science edr tests, could you make a separate test dataset that has the same dimension names as your files? Might require copying the FakeNetCDF4FileHandler2 object. Otherwise I had a couple other small comments. Thanks for doing this.

But the dimension names on Science EDRs are already just a subset of those in standard EDRs.

@djhoese
Copy link
Member

djhoese commented Feb 7, 2020

The dimension names in the tests I wrote use number_of_FORs iirc. Your new files have the Number_Of_CrIS_FORs naming or whatever it is.

@tommyjasmin
Copy link
Contributor Author

The dimension names in the tests I wrote use number_of_FORs iirc. Your new files have the Number_Of_CrIS_FORs naming or whatever it is.

Then we should just change the single ref to check the file name for version num and label the dimension name number_of_FORs only when appropriate.

@djhoese
Copy link
Member

djhoese commented Feb 7, 2020

You mean where the test generates the fake data? Or in the actual reader's file handler? 👍 to the former, not sure on the latter.

@tommyjasmin
Copy link
Contributor Author

tommyjasmin commented Feb 7, 2020 •
edited
Loading

You mean where the test generates the fake data? Or in the actual reader's file handler? 👍 to the former, not sure on the latter.

I was thinking this at the end of the FakeN4 file handler:

        cris_fors_dim_name = 'Number_of_CrIS_FORs'
        pressure_levels_dim_name = 'Number_of_P_Levels'
        """Version 1 EDRs have different dimension names"""
        if ('_v1' in filename):
            cris_fors_dim_name = 'number_of_FORs'
            pressure_levels_dim_name = 'number_of_p_levels'
        convert_file_content_to_data_array(
            file_content, attrs=attrs,
            dims=('z', cris_fors_dim_name, pressure_levels_dim_name))
        return file_content

@djhoese
Copy link
Member

djhoese commented Feb 7, 2020

That works for me.

djhoese
djhoese approved these changes Feb 8, 2020
View reviewed changes
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.

Great! Looks good code-wise. What did we decide for the docstring on the reader module to talk about the differences in the types of files and where they come from?

mraspaud
mraspaud approved these changes Feb 10, 2020
View reviewed changes
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

@tommyjasmin
Copy link
Contributor Author

Great! Looks good code-wise. What did we decide for the docstring on the reader module to talk about the differences in the types of files and where they come from?

We didn't yet Dave :-)
I'm meeting with Brad for a bit tomorrow and I'll get some verbiage straight we can add use for docstring.

djhoese
djhoese approved these changes Feb 20, 2020
View reviewed changes
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.

Awesome! Thanks.

@djhoese djhoese merged commit 7891684 into pytroll:master Feb 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mraspaud mraspaud mraspaud approved these changes

@djhoese djhoese djhoese approved these changes

Assignees

@djhoese djhoese

Labels
component:readers enhancement code enhancements, features, improvements
Projects
None yet
Milestone
v0.20.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@tommyjasmin @coveralls @djhoese @mraspaud