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 not preserving user's requested resolution #523

Merged
merged 3 commits into from
Jun 21, 2023

Conversation

djhoese
Copy link
Member

@djhoese djhoese commented Jun 8, 2023

Closes the new issue described in #517 where the user points out that they requested a resolution of 250.0 but the resulting AreaDefinition does not match that and actually changes between cases. This PR fixes this so in the cases where resolution is provided (and not shape) the resulting area should have pixel_size_x/pixel_size_y very very close to this value except for some floating point precision.

@codecov
Copy link

codecov bot commented Jun 8, 2023

Codecov Report

Merging #523 (9784190) into main (2b94b70) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #523      +/-   ##
==========================================
+ Coverage   94.32%   94.33%   +0.01%     
==========================================
  Files          79       79              
  Lines       12944    12982      +38     
==========================================
+ Hits        12209    12247      +38     
  Misses        735      735              
Flag Coverage Δ
unittests 94.33% <100.00%> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
pyresample/test/test_gradient.py 99.00% <ø> (-0.01%) ⬇️
pyresample/geometry.py 87.53% <100.00%> (+0.02%) ⬆️
pyresample/test/test_geometry_legacy.py 97.68% <100.00%> (+0.03%) ⬆️

... and 1 file with indirect coverage changes

@coveralls
Copy link

coveralls commented Jun 8, 2023

Coverage Status

coverage: 93.939% (+0.09%) from 93.846% when pulling 9784190 on djhoese:bugfix-dynamic-res-preserve into 2b94b70 on pytroll:main.

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, just one placement question

Comment on lines 1134 to 1136
# align corners with pixel resolution
corners[2] = corners[0] + (width - 1) * x_resolution
corners[3] = corners[1] + (height - 1) * y_resolution
Copy link
Member

Choose a reason for hiding this comment

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

doesn't this rather belong to _update_corners_for_full_extent?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not strictly, but I agree it seems like an odd place to put this code compared to the existing version of the code (updating corners when only width/height or x/y resolution were calculated). I don't think it goes in _update_corners_for_full_extents since that is mostly concerned with areas that are the full extent of the projection and only deals with x extents at the moment.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also note the conversion in the related issue that may require other changes to this PR.

@djhoese
Copy link
Member Author

djhoese commented Jun 9, 2023

@mraspaud I have some local changes that make the extents nice round numbers (usually integers, no fractional meters), but it results in a lot of the tests failing because shapes end up being off by 1 with the new logic or extents end up being slightly different. I want to run the code by you before I fix all the failing tests. What do you think:

        if shape:
            height, width = shape
            x_resolution = (corners[2] - corners[0]) * 1.0 / (width - 1)
            y_resolution = (corners[3] - corners[1]) * 1.0 / (height - 1)
            area_extent = (corners[0] - x_resolution / 2,
                           corners[1] - y_resolution / 2,
                           corners[2] + x_resolution / 2,
                           corners[3] + y_resolution / 2)
        else:
            x_resolution, y_resolution = resolution
            half_x = x_resolution / 2
            half_y = y_resolution / 2
            # align extents with pixel resolution
            area_extent = (
                math.floor((corners[0] - half_x) / x_resolution) * x_resolution,
                math.floor((corners[1] - half_y) / y_resolution) * y_resolution,
                math.ceil((corners[2] + half_x) / x_resolution) * x_resolution,
                math.ceil((corners[3] + half_y) / y_resolution) * y_resolution,
            )
            width = int(round((area_extent[2] - area_extent[0]) / x_resolution))
            height = int(round((area_extent[3] - area_extent[1]) / y_resolution))

        return area_extent, width, height

The shape block is basically unchanged. The else is the important part. @yukaribbba your feedback is welcome too.

Basically I kept the width/height calculations essentially the same, but instead of adjusting the corners and then doing half-pixel math, I calculation the extents immediately and round them to the nearest pixel position. This has two benefits to the existing code:

  1. I'm not adding pixels only to the right side of the area, but adding space on both sides.
  2. The "round" extents mean that there is very little chance of floating point precision issues adding 0.000000005 to the resulting resolution. Especially in metered projections where we likely have no fractional parts to the extents.

@yukaribbba
Copy link

I have no problem with this. Don't forget to exclude decimals.

@djhoese
Copy link
Member Author

djhoese commented Jun 10, 2023

Exclude decimals?

@yukaribbba
Copy link

Yeah I mean decimal degrees. Wait... no need for that.

@djhoese
Copy link
Member Author

djhoese commented Jun 10, 2023

Yeah we should be fine since we're doing everything based on resolution.

@mraspaud
Copy link
Member

@djhoese I'm good with that!

@djhoese djhoese added this to the v1.27.1 milestone Jun 19, 2023
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 e406472 into pytroll:main Jun 21, 2023
30 checks passed
@djhoese djhoese deleted the bugfix-dynamic-res-preserve branch June 21, 2023 13:09
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.

EWA resampling in 1.27 slows down four times than 1.26.1
4 participants