coecms / frontdetection

Apache License 2.0
2 stars 2 forks source link

Refactor #14

Open HoWol76 opened 3 years ago

HoWol76 commented 3 years ago

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

HoWol76 commented 3 years ago

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:

Could someone please have a look at my new implementation?

ccarouge commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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.