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

Bug: to_radians applied too late in model.py grid_sfc_area? #54

Closed
spencerkclark opened this issue Jan 27, 2016 · 4 comments
Closed

Bug: to_radians applied too late in model.py grid_sfc_area? #54

spencerkclark opened this issue Jan 27, 2016 · 4 comments
Labels

Comments

@spencerkclark
Copy link
Collaborator

I hadn't pulled from the develop branch in a while, but I just did this morning. In doing some calculations involving surface area I noticed some things were off. In printing a sum of the surface area returned I got a value of:

<xarray.DataArray 'sfc_area' ()>
array(2.9224513997287056e+16)

This is too large by a factor of 180.0 / pi, hinting that it stems from not converting from degrees to radians.

I think the issue stems from line 194 in model.py:

dlon = to_radians(cls.diff_bounds(lon_bounds, lon))

In my case, but I think in most cases, the width of gridboxes in longitude in degrees is less than 4 pi. This causes utils.to_radians to pass the widths back unchanged. I think a solution here would be to just apply to_radians to lon_bounds (instead of after diff_bounds):

dlon = cls.diff_bounds(to_radians(lon_bounds), lon)

Luckily this shouldn't change any results from area averaging (since it's off by a constant factor), but important to get right nonetheless.

@spencerahill
Copy link
Owner

Yikes, good catch. I'll fix this right now.

@spencerkclark
Copy link
Collaborator Author

Thanks!

@spencerahill
Copy link
Owner

02a8a8f should resolve this. I implemented the switch you suggested and also improved the to_radians function itself, both the threshold for conversion and by trying to use the embedded 'units' metadata.

This and the other conversion methods could definitely be improved, potentially even a conversions class or module created.

(Yet another reminder of the need for formal automated tests, both so that we would have caught this in the first place and that I should have added some for the new code in order to be sure it actually works! One day...)

Let me know if there's still any problems, otherwise please close.

@spencerkclark
Copy link
Collaborator Author

This looks great; things are working as expected now. I like the additional improvements to to_radians! Thanks again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants