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 a YAMLReader to pad segmented geo data #977

Merged
merged 33 commits into from Dec 10, 2019

Conversation

mraspaud
Copy link
Member

@mraspaud mraspaud commented Nov 26, 2019

This PR adds a class that knows how to pad segmented geo data

@mraspaud mraspaud added enhancement code enhancements, features, improvements component:readers labels Nov 26, 2019
@mraspaud mraspaud added this to Planned in PCW Copenhagen 2019 via automation Nov 26, 2019
@coveralls
Copy link

coveralls commented Nov 26, 2019

Coverage Status

Coverage increased (+0.1%) to 86.992% when pulling 0831cd2 on mraspaud:feature-geo-padding into 071e34b on pytroll:master.

@codecov
Copy link

codecov bot commented Nov 26, 2019

Codecov Report

Merging #977 into master will decrease coverage by 0.11%.
The diff coverage is 97.78%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #977      +/-   ##
==========================================
- Coverage    87.1%   86.99%   -0.12%     
==========================================
  Files         181      181              
  Lines       27635    27752     +117     
==========================================
+ Hits        24072    24143      +71     
- Misses       3563     3609      +46
Impacted Files Coverage Δ
satpy/tests/test_yaml_reader.py 99.06% <100%> (+0.54%) ⬆️
satpy/readers/yaml_reader.py 94.17% <94.69%> (-0.07%) ⬇️
satpy/tests/reader_tests/test_utils.py 79.69% <0%> (-18.15%) ⬇️
satpy/readers/acspo.py 86.25% <0%> (-7.96%) ⬇️
satpy/readers/netcdf_utils.py 94.02% <0%> (-5.98%) ⬇️
satpy/readers/seviri_l1b_native.py 67.24% <0%> (-4.41%) ⬇️
satpy/readers/utils.py 75.23% <0%> (-4.33%) ⬇️
satpy/readers/li_l2.py 23.07% <0%> (-2.79%) ⬇️
satpy/tests/reader_tests/test_netcdf_utils.py 93.24% <0%> (-1.44%) ⬇️
satpy/readers/caliop_l2_cloud.py 31.57% <0%> (-1.18%) ⬇️
... and 27 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 a0844ed...7328b73. Read the comment docs.

@pnuu
Copy link
Member

pnuu commented Nov 27, 2019

I think everything else works, but when the first segment is missing there's a crash. With MSG/SEVIRI HRIT data:
ValueError: conflicting sizes for dimension 'y': length 3248 on 'y' and length 3712 on 'acq_time'

@pnuu
Copy link
Member

pnuu commented Nov 27, 2019

And when the first segment is missing from Himawari-8/AHI HRIT:
ValueError: conflicting sizes for dimension 'y': length 5500 on <this-array> and length 4950 on 'y'.

@pnuu pnuu marked this pull request as ready for review December 2, 2019 11:16
@pnuu
Copy link
Member

pnuu commented Dec 2, 2019

The tests are failing because something in packages that Conda installs has changed.

I've tested the padding and found it working for the following data:

  • SEVIRI HRIT via seviri_l1b_hrit reader - 0 deg, RSS and IOCD services
  • Himawari-8 HRIT via ahi_hrit reader
  • GOES-13/15 HRIT via goes-imager_hrit reader

Both saving directly as full disk data and resampling the data works, as long as even one segment is available.

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.

Very cool. I have a couple concerns about the naming. "Collection" seems like a very generic name for this. In the future I'd like to add a "tiled" reader, but the StackedAreaDefinition doesn't support this idea. This "Collection" reader only supports stacking segments, right? What do you think about a "Segmented" reader name or "Stacked"?

Also, this means that people always get a full disk sized array right? Is there a way users can disable this? Like what if they only provide the segments for the top half of the full disk? Could a keyword argument be added to the Reader?

@pnuu
Copy link
Member

pnuu commented Dec 4, 2019

Segmented does sound better. And yes, it supports stacking and padding only GEO segments together.

Yeah, the data are always returned as full disk representations. I'll have a look at adding the kwarg, and how it would actually be used when creating the Scene. Is it something like scn = Scene(..., reader_kwargs={'pad_data': False})?

@djhoese
Copy link
Member

djhoese commented Dec 4, 2019

Yeah I think that should work.

@pnuu
Copy link
Member

pnuu commented Dec 5, 2019

OK, disabling is now implemented. It needs to be done at load time:

scn.load([10.8], pad_data=False)

Unfortunately resampling when segments are missing is broken also in master branch (discussing this on slack). With padding to full disk (the default behaviour with this PR) the result is as expected.

# Pad missing segments between the first available and expected
area_defs = _pad_later_segments_area(file_handlers, dsid)
if not pad_data:
return _load_area_def(dsid, file_handlers)
Copy link
Member Author

Choose a reason for hiding this comment

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

Why not use super and make a extra function ?

Copy link
Member

Choose a reason for hiding this comment

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

Because:

  1. it didn't need to be a method
  2. @staticmethod would've caused other problems
  3. I couldn't get it to work with super()

In short, this was the easiest and fastest way to get it to work 😁

@pnuu pnuu changed the title Add a CollectionFileReader to pad segmented geo data Add a YAMLReader to pad segmented geo data Dec 5, 2019
@pnuu pnuu merged commit 6451afe into pytroll:master Dec 10, 2019
PCW Copenhagen 2019 automation moved this from Planned to Done Dec 10, 2019
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
Development

Successfully merging this pull request may close these issues.

Pad all geostationary L1 data to full disk area
4 participants