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

Fix DynamicAreaDefinition resolution handling for incomplete projection definitions #424

Merged
merged 3 commits into from
Feb 18, 2022

Conversation

mraspaud
Copy link
Member

It came up as a use case that sometimes it might be interesting to fix the resolution of a partially defined DynamicAreaDefinition (that is thus optimized upon freezing). Until this PR, the resolution was always computed in this case from the size of the lon/lat arrays, and any user-defined resolution was just ignored. This PR allows for example having a area like:

omerc_bb_1000:
  description: Oblique Mercator Bounding Box for Polar Overpasses
  projection:
    ellps: sphere
    proj: omerc
  optimize_projection: True
  resolution: 1000
  • Tests added

@mraspaud mraspaud added the bug label Feb 17, 2022
@mraspaud mraspaud self-assigned this Feb 17, 2022
@codecov
Copy link

codecov bot commented Feb 17, 2022

Codecov Report

Merging #424 (c4388fd) into main (1928289) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #424   +/-   ##
=======================================
  Coverage   93.89%   93.89%           
=======================================
  Files          65       65           
  Lines       11130    11169   +39     
=======================================
+ Hits        10450    10487   +37     
- Misses        680      682    +2     
Flag Coverage Δ
unittests 93.89% <100.00%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
pyresample/area_config.py 90.55% <100.00%> (ø)
pyresample/geometry.py 87.04% <100.00%> (+0.03%) ⬆️
pyresample/test/test_geometry.py 99.47% <100.00%> (+0.01%) ⬆️
pyresample/test/test_utils.py 100.00% <100.00%> (ø)
pyresample/ewa/dask_ewa.py 90.16% <0.00%> (-0.82%) ⬇️

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 1928289...c4388fd. Read the comment docs.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.004%) to 93.717% when pulling c4388fd on mraspaud:dynamic-resolution into 1928289 on pytroll:main.

@djhoese
Copy link
Member

djhoese commented Feb 17, 2022

This should be clarified, if anything in the title of this PR, that I think this is only an issue with invalid projections. For some reason your omerc projection isn't pyproj friendly and reaches that exception block where you've made the fix. We even have a test case where resolution is used for an LCC projection and I'm pretty sure I use DynamicAreaDefinitions with resolution just fine in Polar2Grid.

test_dynamic_resolution:
description: Dynamic with resolution specified in meters
projection:
proj: lcc
lon_0: -95.0
lat_0: 25.0
lat_1: 25.0
ellps: WGS84
resolution: [1000, 1000]

@mraspaud
Copy link
Member Author

ah, interesting! Maybe the optimize_projection should be renamed to fix_projection or something.
The omerc projection needs indeed more information to be complete, and this is what that parameter triggers the computation of.

@djhoese
Copy link
Member

djhoese commented Feb 17, 2022

I haven't looked at the code in a while, but yeah there could be some more flexibility builtin with providing parameters to a PROJ.4 string or something. Maybe "freeze_projection" ("_string") or something along those lines.

@mraspaud
Copy link
Member Author

I'm looking at this renaming, but it's going to be quite some work to set up all the deprecation warnings and such.

Re your original question, the projection is not invalid as much as incomplete. Is there something you want me to correct or clarify in the doc or in this PR's title or comment?

@djhoese
Copy link
Member

djhoese commented Feb 17, 2022

Regarding deprecation warnings, for what? The renaming of that function? Just leave it for now.

Regarding clarifying, maybe just retitle this PR to something like "Fix DynamicAreaDefinition resolution handling for incomplete projection definitions" or something 🤷‍♂️

@mraspaud
Copy link
Member Author

Regarding deprecation warnings, for what? The renaming of that function? Just leave it for now.

Renaming optimize_projection to freeze_projection

@mraspaud mraspaud changed the title Allow optimized DynamicAreaDefinition to be passed a resolution Fix DynamicAreaDefinition resolution handling for incomplete projection definitions Feb 18, 2022
@mraspaud
Copy link
Member Author

@djhoese do you think this is good to merge?

@djhoese
Copy link
Member

djhoese commented Feb 18, 2022

Go for it

@mraspaud mraspaud merged commit 8658d17 into pytroll:main Feb 18, 2022
@mraspaud mraspaud deleted the dynamic-resolution branch February 18, 2022 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants