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

Implement generic set_orientation method for geostationary datasets #1194

Merged
merged 23 commits into from Jul 23, 2020

Conversation

ameraner
Copy link
Member

@ameraner ameraner commented May 8, 2020

This PR implements a generic set_orientation Scene method. This method allows to flip a dataset to a user-defined orientation, without having to resample the Scene.
Due to geometry constraints, this is only valid for Datasets in geos projection.

  • Tests added
  • Tests passed
  • Passes flake8 satpy
  • Fully documented

@ghost
Copy link

ghost commented May 8, 2020

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

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

@coveralls
Copy link

coveralls commented May 8, 2020

Coverage Status

Coverage increased (+0.03%) to 90.037% when pulling 7b2419f on ameraner:implement_orientation_method into 0b986b2 on pytroll:master.

@mraspaud
Copy link
Member

mraspaud commented May 8, 2020

As mentioned in the video chat, it would be nice to allow omitting the dataset list if we want to set the orientation to all datasets.

@ameraner ameraner added this to In progress in PCW Spring 2020 May 8, 2020
@codecov
Copy link

codecov bot commented May 8, 2020

Codecov Report

Merging #1194 into master will increase coverage by 0.03%.
The diff coverage is 96.79%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1194      +/-   ##
==========================================
+ Coverage   90.00%   90.03%   +0.03%     
==========================================
  Files         218      218              
  Lines       31352    31498     +146     
==========================================
+ Hits        28219    28360     +141     
- Misses       3133     3138       +5     
Impacted Files Coverage Δ
satpy/readers/yaml_reader.py 95.25% <93.82%> (-0.25%) ⬇️
satpy/tests/test_yaml_reader.py 99.81% <100.00%> (+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 0b986b2...7b2419f. Read the comment docs.

@ameraner
Copy link
Member Author

ameraner commented May 8, 2020

@mraspaud indeed, just implemented it! Along with support for StackedAreaDefinitions.

satpy/scene.py Outdated Show resolved Hide resolved
@ameraner ameraner marked this pull request as ready for review June 4, 2020 16:57
Copy link
Member

@sfinkens sfinkens left a comment

Choose a reason for hiding this comment

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

I have a couple of questions, but good work so far!

satpy/readers/yaml_reader.py Outdated Show resolved Hide resolved
logger.info("Dataset is not a geographical dataset and cannot be flipped.")
return dataset

if dataset.attrs['area'].proj_dict['proj'] != 'geos':
Copy link
Member

Choose a reason for hiding this comment

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

Is is possible to check the area's CRS here instead of the proj_dict?

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess so yes. What is the advantage of doing so?

Copy link
Member

Choose a reason for hiding this comment

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

we're slowly migrating to using crs more instead of proj dicts

Copy link
Member

Choose a reason for hiding this comment

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

also this here doesn't cover the projections defined by epsg code

Copy link
Member Author

Choose a reason for hiding this comment

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

Understood. So I changed to:

if dataset.attrs['area'].crs.coordinate_operation.method_name not in ['Geostationary Satellite (Sweep Y)',
                                                                                                               'Geostationary Satellite (Sweep X)']:

Copy link
Member

Choose a reason for hiding this comment

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

👍 @mraspaud Can we be sure that the area def has a crs property or do we need to fallback to the proj_dict?

Copy link
Member

Choose a reason for hiding this comment

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

Iirc the crs is a property of the areadef so always there

Copy link
Member

Choose a reason for hiding this comment

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

crs should always exist if they are using a new-ish version of pyproj (2.0+).

satpy/readers/yaml_reader.py Outdated Show resolved Hide resolved
satpy/readers/yaml_reader.py Outdated Show resolved Hide resolved
@ameraner
Copy link
Member Author

Hi all,
I finally made progress on this PR. I added tests (thanks to @mraspaud for the support), split the _set_orientation method into smaller functions and added the documentation.

I noticed that the loading of ancillary variables was loosing the load kwargs, so I added that functionality as well. This is needed e.g. for the loading of pixel_quality maps in the FCI reader.

So I would be very grateful if you could review it (again)! Thanks a lot in advance!

@ameraner ameraner requested a review from sfinkens July 22, 2020 12:41
PCW Spring 2020 automation moved this from In progress to Ready to merge Jul 23, 2020
Copy link
Member

@sfinkens sfinkens left a comment

Choose a reason for hiding this comment

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

Nice work, and a very useful feature!

@sfinkens
Copy link
Member

Oh, but some fci tests are still failing

@ameraner
Copy link
Member Author

Oh, but some fci tests are still failing

Yes, working on it! A hack that I used in my tests is getting back at me and is influencing other tests when running together.
But thanks for your review so far!

@ameraner
Copy link
Member Author

Tests are fixed and Travis is finally happy :)

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, thanks for adding this nice feature. One question though: did you check that loading the ancillary variables still works and actually flips the data too?

@ameraner
Copy link
Member Author

ameraner commented Jul 23, 2020

Yes I checked using this:

path_to_testdata = '/your/path/to/testdata/folder/'
scn = Scene(filenames=glob(path_to_testdata + "/*BODY*.nc"), reader=['fci_l1c_fdhsi'])

# load channel
scn.load(['ir_105'], upper_right_corner='NE')
plt.figure()
plt.imshow(scn['ir_105'].values)

# pixel quality was loaded automatically together with the channel as ancillary variable
# extract values and remove FillValue
pixel_quality = scn['ir_105'].ancillary_variables[0].values.astype('float32')
pixel_quality[pixel_quality == 255] = np.nan
plt.figure()
plt.imshow(pixel_quality)

# check that also the direct loading works
scn.load(['ir_105_pixel_quality'], upper_right_corner='NE')
pixel_quality_d = scn['ir_105_pixel_quality'].values.astype('float32')
pixel_quality_d[pixel_quality_d == 255] = np.nan
plt.figure()
plt.imshow(pixel_quality_d)

the code yields these images:

image

demonstrating that all variables are loaded and flipped correctly.

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

@mraspaud mraspaud merged commit ffd10f5 into pytroll:master Jul 23, 2020
PCW Spring 2020 automation moved this from Ready to merge to Done Jul 23, 2020
@ameraner ameraner deleted the implement_orientation_method branch December 7, 2020 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

6 participants