Uh oh!
There was an error while loading. Please reload this page.
Remove addition of period from wrap_lons - #3994
Conversation
akuhnregnier
commented
Feb 10, 2021
I have now signed the SciTools CLA. |
a0c8524 to
958ae5aComparewjbenfold
commented
Nov 10, 2021
I previously saw this when working on Indeed, checking your test values against the true value, we find that you've reduced the error: I therefore think it's sensible to update the test value. Given that main has moved on in the time since this was last looked at, it will be necessary to rebase this onto main (let us know if you'd like a hand with that). |
958ae5a to
64e544aComparewjbenfold
commented
Nov 11, 2021
Having slept on this, and remembered the discussion from #3993, I think that I'd prefer if the fix for this test was to check that the values are close - this could either be done with Sorry for not coming up with this originally |
64e544a to
42330c2Compareakuhnregnier
commented
Nov 11, 2021
Ok, no worries - I just pushed an update that should follow this advice. |
Updated affected tests using assertArrayAllClose following SciTools#3993.
64ceedc to
196efaaComparewjbenfold
commented
Nov 15, 2021
I'm happy with this now, with the exception of the tests failing before they even run (though I can run them locally and they all pass). I'll see if I can fix this, as I can't see any changes on this branch that should cause it |
akuhnregnier
commented
Nov 15, 2021
Ok, that's great. Thank you for looking into the failing tests, as this was confusing me as well. |
trexfeathers
commented
Nov 16, 2021
I'm just gonna try closing this and re-opening it again... |
trexfeathers
commented
Nov 16, 2021
No luck! |
trexfeathers
commented
Nov 16, 2021
So a916cb198538d2 are identical, but the one on this PR has the original CI failure and the one on a new PR is fine. @akuhnregnier you obviously deserve full credit for the work you've put in, so I'm going to close #4420 and I suggest making a new PR with your original changes, plus the latest changes from |
akuhnregnier
commented
Nov 16, 2021
Ok, I will do that and try again! |
akuhnregnier
commented
Nov 16, 2021
The new PR is now here: #4421 |
🚀 Pull Request
Description
I have removed the addition of
period * 2from thewrap_lons()function after considering if this is really needed.The only additional (see #3993) test that fails following the change is:
This looks like a floating point error to me.
Expand error log (with package versions)
Sanity check
The following snippet directly compares the function before (
factor=2) and after (factor=0) and finds only marginal relative differences in the output:In [1]: importnumpyasnp ...: ...: ...: def wrap_lons(lons, base=-180, period=360, factor=2): ...: return ((lons - base + period * factor) % period) + base ...: ...: ...: values = np.linspace(-720, 720, int(1e6)) ...: ...: w_old = wrap_lons(values, factor=2) ...: w_new = wrap_lons(values, factor=0) ...: div_zero_mask = np.abs(w_old) > 0 ...: rel_diffs = (w_old[div_zero_mask] - w_new[div_zero_mask]) / w_old[div_zero ...: _mask] ...: print("Maximum |relative diff|:", np.max(np.abs(rel_diffs))) Maximum |relative diff|: 1.5789822785557709e-10If I have missed something here please let me know.
Consult Iris pull request check list