Skip to content

Fix bug in get_gridcellarea function in util/coordinates#380

Merged
emanuel-schmid merged 2 commits intodevelopfrom
hotfix/grid_cell_area
Mar 24, 2022
Merged

Fix bug in get_gridcellarea function in util/coordinates#380
emanuel-schmid merged 2 commits intodevelopfrom
hotfix/grid_cell_area

Conversation

@carmensteinmann
Copy link
Copy Markdown
Collaborator

Hello!

I came across an issue with the unit / order of magnitude of this function.
Assuming that the max grid cell area should be 2500km^2 (50x50km), I am not sure why we had added *100 in the first place - or am I getting mixed up?

Thanks for having a look!

Best, Carmen

area = (ONE_LAT_KM * resolution)**2 * np.cos(np.deg2rad(lat)) * 1000000
else:
area = (ONE_LAT_KM * resolution)**2 * np.cos(np.deg2rad(lat)) * 100
area = (ONE_LAT_KM * resolution)**2 * np.cos(np.deg2rad(lat))
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This correction makes sense to me. I have also double checked with some example coordinates using this external tool: https://www.engr.scu.edu/~emaurer/tools/calc_cell_area_cgi.pl and it looks alright.

@bguillod
Copy link
Copy Markdown
Collaborator

The factor 100 could be for a conversion from km2 to ha. Could it be that somewhere else in Climada this unit is (wrongly) expected? Might be good to check everywhere the function is used before merging, in case you haven’t yet done so

@carmensteinmann
Copy link
Copy Markdown
Collaborator Author

@bguillod thanks a lot for that comment. Thinking about it, probably the crop production data we initially wrote this function for has the unit t/ha. I'll check and add the unit option ha.

@sameberenz
Copy link
Copy Markdown
Collaborator

@bguillod thanks a lot for that comment. Thinking about it, probably the crop production data we initially wrote this function for has the unit t/ha. I'll check and add the unit option ha.

Sounds reasonable, in that case you should check how the funciton is used in teh crop module and if this needs to be aligned

@bguillod
Copy link
Copy Markdown
Collaborator

@bguillod thanks a lot for that comment. Thinking about it, probably the crop production data we initially wrote this function for has the unit t/ha. I'll check and add the unit option ha.

Sounds reasonable, in that case you should check how the funciton is used in teh crop module and if this needs to be aligned

Actually a quick search on all calls to that function might be worth it, to make sure you don’t miss other cases which might expect units other than km2 or m2.

@ChrisFairless
Copy link
Copy Markdown
Collaborator

yeah it looks like it's called once in climada_petals/entity/exposures/crop_production.py

@carmensteinmann
Copy link
Copy Markdown
Collaborator Author

Yes, thanks a lot everyone! I changed the default of the function to ha (as it is used in crop production) and added the calculation in km2.

@chahank chahank requested review from ChrisFairless and removed request for chahank March 24, 2022 08:10
@sameberenz sameberenz removed their assignment Mar 24, 2022
@emanuel-schmid emanuel-schmid merged commit 334f290 into develop Mar 24, 2022
@emanuel-schmid emanuel-schmid deleted the hotfix/grid_cell_area branch March 24, 2022 12:40
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.

5 participants