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

Refactor #14

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Refactor #14

wants to merge 9 commits into from

Conversation

HoWol76
Copy link

@HoWol76 HoWol76 commented Sep 9, 2021

Removed duplication between wetbulb and dewpoint, created a few more elemental functions that can be tested independently, created a unittesting script. resolves #1

@HoWol76
Copy link
Author

HoWol76 commented Sep 14, 2021

Since I made this original pull request, there has been a complete rewrite of the zeropoints function.

Unfortunately, this has come with a slight change in behaviour:

  • It now finds zero-transitions in the last row and column of the data
  • Zero points on the grid are now returned at the beginning of the list.

Could someone please have a look at my new implementation?

@ccarouge
Copy link
Member

ccarouge commented Sep 15, 2021

I had a look. I've added some json outputs from both version and compared using sorted DataFrames. I can confirm that the 2 versions give the same results. Except the original code did not find the zero-transitions in the last row and column of the data. This is because it looped on range(0, n-1) on each direction. For the moment, my testing is in branch test_newzeropoints

I have asked Malcolm if the change of behaviour is acceptable.

@ccarouge
Copy link
Member

Reply from Malcolm:

I'm looking at the original code, and yep, it was an oversight (or at best a semi-intentional error). I think I had it like that because of the comparison at the end of the loop, but of course that should still be done for things at the edge of the lat/lon range. I'd keep the extra points in Holger's version, especially if we're not dealing with global data.

So the version from Holger is acceptable from the point of view of results. Just need a review now 😄

@ccarouge
Copy link
Member

Tests come back with some Warning:

/scratch/w35/ccc561/frontdetection/fronts.py:195: PendingDeprecationWarning: xarray.ufuncs will be deprecated when xarray no longer supports versions of numpy older than v1.17. Instead, use numpy ufuncs directly.
    mag = xr.ufuncs.sqrt(dtdy ** 2 + dtdx ** 2)

We'll want to remove the calls to xr.ufuncs.

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.

use dewpoint function in wetbulb
2 participants