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

Add MOD06 support to 'modis_l2' reader #812

Merged
merged 7 commits into from
Jun 1, 2020

Conversation

BENR0
Copy link
Collaborator

@BENR0 BENR0 commented Jun 11, 2019

Fixes the bug where the file_key tag of a dataset in the yaml file was not used when a dataset where no bit manipulation is required is loaded.

Additionally some datasets of the mod06_l2 files where added to the yaml file.

Closes #1200

…" case

Fixes the bug where the get dataset function does not respect the
file_key tag in the yaml file of the reader when no byte manipulation is
required.

Adds mod06 dataset to the yaml file.
@coveralls
Copy link

coveralls commented Jun 11, 2019

Coverage Status

Coverage increased (+5.6%) to 89.632% when pulling f1c32cc on BENR0:modis_l2_reader into 44239a0 on pytroll:master.

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.

I wonder if this could benefit from the available_datasets method being defined. Complicated, yes, but you'd be able to add any field in the file.

Edit: Oops https://satpy.readthedocs.io/en/latest/api/satpy.readers.html#satpy.readers.file_handlers.BaseFileHandler.available_datasets

coordinates: [longitude, latitude]

cloud_effective_radius_1km:
name: cloud_effective_radius_1km
Copy link
Member

Choose a reason for hiding this comment

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

Why does this name have the resolution in it when the others don't?

Copy link
Collaborator Author

@BENR0 BENR0 Jun 11, 2019

Choose a reason for hiding this comment

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

I am not sure right now if this is true for that specific dataset but some of them have 5km and 1km datasets in the file and are named that way then too. I will check up on that. Eventually if only 1km is present the 1km can be removed from the name.

Regarding the available_datasets: hadn't thought about that but I guess that would be nice since the reader then probably would support most, if not all, level 2 files without the need of updating the yaml file.
Due to preparations for a talk I have to give next week I am a little short on time so I don't have capacity to have a look at it right away. I wonder if for the time being it would be possible to merge the mod06 addition since we want to use those files in a course we are giving right now and it would be nice if the students could just use the module installed from the master instead of my branch (all of them are python beginners and they are struggling enough already with the topics covered)? I would then do another PR when I come up with a general solution using the available_datasets method.

@djhoese djhoese changed the title Modis l2 reader Add MOD06 support to 'modis_l2' reader Jun 11, 2019
@mraspaud mraspaud added component:readers enhancement code enhancements, features, improvements labels Jun 11, 2019
@mraspaud
Copy link
Member

@BENR0 @djhoese what's the status on this ? Are we waiting for more to be done ?

@djhoese
Copy link
Member

djhoese commented Sep 18, 2019

I'm ok with not requiring available_datasets to be implemented to merge this. However, there are some things we need to worry about if/when that is done in the future:

  1. The dataset names should most likely match the names of the variables in the file including capitalization. This seems to be the way we are going for readers like this which can take any variable from the input file. I would also be OK with a general of lowercasing the in-file variable name to get the dataset name in Satpy which seems to be the general rule in this PR.

  2. The dataset names that have a 1km and a 5km version should be merged to be one dataset:

    surface_temperature:
      name: surface_temperature
      file_type: mod06_hdf
      coordinates: [longitude, latitude]
      resolution:
        1000:
          file_key: surface_temperature_1000
        5000:
          file_key: Surface_Temperature

@BENR0 Had also mentioned needing to check on the files and what variables are available for different resolutions.

@BENR0
Copy link
Collaborator Author

BENR0 commented Sep 20, 2019

I totally forgot about this. Regarding the refactoring @djhoese mentions above I probably have time later today and update the PR accordingly then. I haven't come around to think about the available_datasets implementation.

I would also be fine if this is merged after the reworks mentioned but note that not all datasets that are available in the mod_06 file are implemented in this PR. But I guess for someone needing one of the implemented ones there would be added value.

@codecov
Copy link

codecov bot commented Sep 23, 2019

Codecov Report

Merging #812 into master will increase coverage by 6.57%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #812      +/-   ##
==========================================
+ Coverage   83.06%   89.63%   +6.57%     
==========================================
  Files         163      200      +37     
  Lines       23592    29601    +6009     
==========================================
+ Hits        19596    26532    +6936     
+ Misses       3996     3069     -927     
Impacted Files Coverage Δ
satpy/readers/modis_l2.py 98.52% <100.00%> (+0.02%) ⬆️
satpy/__init__.py 85.71% <0.00%> (-9.03%) ⬇️
satpy/readers/seviri_base.py 90.90% <0.00%> (-7.81%) ⬇️
satpy/readers/nucaps.py 89.04% <0.00%> (-5.08%) ⬇️
satpy/readers/viirs_edr_active_fires.py 90.32% <0.00%> (-3.16%) ⬇️
satpy/tests/utils.py 95.23% <0.00%> (-2.22%) ⬇️
satpy/writers/scmi.py 72.52% <0.00%> (-1.91%) ⬇️
satpy/readers/generic_image.py 93.33% <0.00%> (-1.04%) ⬇️
satpy/tests/writer_tests/test_cf.py 98.53% <0.00%> (-0.39%) ⬇️
... and 190 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 a5733f3...f1c32cc. Read the comment docs.

@BENR0
Copy link
Collaborator Author

BENR0 commented Sep 23, 2019

I updated the yaml file with some more datasets contained in the mod06 files and combined the datasets which are available in multiple resolutions as suggested by @djhoese . There are still some datasets missing (for example cloud_mask which is a subset of the cloud mask in the mod35_l2 files).

I lower cased all variables names (in the file most of them are indeed capitalized but some are lower case. Personally I like the lower case version but I would also be fine if it is decided to capitalize.

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. If @djhoese is ok with the last changes, we can merge this.

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 except the tests are failing. As I mentioned before this would really benefit from available_datasets but refactoring the code to do that isn't necessary for this to be merged.

@mraspaud
Copy link
Member

Looks like #913 is tackling the available_datasets. I'll have a quick look at why travis fails

@mraspaud mraspaud merged commit d18e72a into pytroll:master Jun 1, 2020
@BENR0 BENR0 deleted the modis_l2_reader branch December 21, 2020 17:24
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.

5 participants