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

Spatial aggregation fix #317

Merged
merged 2 commits into from
Jan 22, 2023
Merged

Spatial aggregation fix #317

merged 2 commits into from
Jan 22, 2023

Conversation

dbkhout
Copy link
Contributor

@dbkhout dbkhout commented Dec 22, 2022

Although utils.dimension will be deprecated in pysteps-v2 (#223), I'm proposing a small update of its aggregate_fields_space function to fix a potential error.

The snippet below is currently used in both aggregate_fields_time and aggregate_fields_space to select the appropriate aggregation method based on the precipitation unit.

if unit == "mm/h":
    method = "mean"
elif unit == "mm":
    method = "sum"
else:
    raise ValueError(
        "can only aggregate units of 'mm/h' or 'mm' " + "not %s" % unit
    )

While this is indeed the correct approach for the temporal aggregation, it leads to unexpected results* for the spatial counterpart. Hence, I'm suggesting to update aggregate_fields_space to use 'mean' aggregation for both 'mm/h' and 'mm' units, as per this PR.


(*) illustration: corresponding example from pystep's test suite
Assuming grid cell dimensions in meter and 1 mm = 1 L/m², the total rainfall volume over the complete field is:

  • 10 * 10 * 1 m² * 1 L/m² = 100 L before aggregation (10x10 grid of 1x1 cells with value 1 mm)
  • 5 * 5 * 4 m² * 4 L/m² = 400 L after aggregation (5x5 grid of 2x2 cells with value 4 mm)

while this value is expected to remain unaffected by the aggregation.

Daan Buekenhout added 2 commits December 20, 2022 20:08
Correct spatial aggregation method to 'mean' for both 'mm/h' and 'mm' precipitation units.
Correct spatial aggregation test.
Copy link
Member

@dnerini dnerini left a comment

Choose a reason for hiding this comment

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

Good catch, @dbkhout ! Aggregating mm in space should use the mean instead of the sum, you're absolutely right.

Thanks for the bug fix and apologies for the very slow response.

@dnerini dnerini merged commit 65e2fe8 into pySTEPS:master Jan 22, 2023
@dbkhout dbkhout deleted the spatial-aggregation-fix branch January 22, 2023 13:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants