-
Notifications
You must be signed in to change notification settings - Fork 64
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
fixes for 'lat=0' and 0 time series issues #62
base: master
Are you sure you want to change the base?
Conversation
utide/confidence.py
Outdated
Duu = Puu[c] * Duu / np.trace(Duu) | ||
Duu = Puu[c] * Duu | ||
if np.trace(Duu) != 0. : | ||
Duu = Duu / np.trace(Duu) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't compute np.trace
twice and we can do the Duu
division in place:
trace = np.trace(Duu)
if trace != 0. :
Duu /= trace
if abs(lat) < 5: | ||
if lat == 0: | ||
lat = 5 | ||
elif abs(lat) < 5: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
utide/confidence.py
Outdated
@@ -252,7 +252,9 @@ def _confidence(coef, cnstit, opt, t, e, tin, elor, xraw, xmod, W, m, B, | |||
varcov_mCw[c, :2, :2] = Duu | |||
|
|||
if not opt.white: | |||
Duu = Puu[c] * Duu / np.trace(Duu) | |||
Duu = Puu[c] * Duu |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a trailing space after Duu
.
utide/confidence.py
Outdated
@@ -252,7 +252,9 @@ def _confidence(coef, cnstit, opt, t, e, tin, elor, xraw, xmod, W, m, B, | |||
varcov_mCw[c, :2, :2] = Duu | |||
|
|||
if not opt.white: | |||
Duu = Puu[c] * Duu / np.trace(Duu) | |||
Duu = Puu[c] * Duu | |||
if np.trace(Duu) != 0. : |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a whitespace before :
.
utide/confidence.py
Outdated
Duu = Puu[c] * Duu | ||
trace = np.trace(Duu) | ||
if trace != 0.: | ||
Duu = Duu / trace |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason to not use the compact in place Duu /= trace
?
I'll leave this here so @efiring and @wesleybowman can take a second look before merging. Thanks @apatlpo for the PR! |
Two real problems have been identified, but based on a quick look, I'm not sure the solutions are good ones.
An oddity related to the latter point is that the code is using conditional statements involving only values in |
I think one of my potential objections is invalid, but the main one--the odd behavior within 5 degrees of the equator--still needs attention. I see that the behavior is inherited from t_tide, which includes some explanatory comments but still doesn't make sense to me. It implies a discontinuity at the equator. |
Here are the explanatory comments:
Maybe there is more in Foreman's code It may be more simple to ask also experts. |
After a brief discussion and a lengthier email, Richard Ray's opinion is that ''bringing latitude dependence in the calculation of nodal modulations, as well as in the inference of minor tides is not a good idea''. I asked him for a good reference in order to know what we are supposed to do instead, here is his reply:
|
@apatlpo Thank you, I hope he can provide a reference we can work with. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Is it clear that the problem of zero data occurs only in the non-white-noise case, so this is the ideal place to intercept it? Regardless, I think it might be best to just raise an exception when obviously useless data is provided. A sanity check could be done early on, as an alternative to checking the trace of Duu.
- There must be a better way to handle the singularity at lat=0. We just have to find it. It's possible a horrible ad-hoc fix something like the one here will be necessary temporarily. Rather than having a jump at the equator, I suspect it would make more sense to have a linear transition in
rr
values from 5N to 5S. But I don't understand the math and physics behindrr
right now, so I don't know how to handle it.
@@ -252,7 +252,10 @@ def _confidence(coef, cnstit, opt, t, e, tin, elor, xraw, xmod, W, m, B, | |||
varcov_mCw[c, :2, :2] = Duu | |||
|
|||
if not opt.white: | |||
Duu = Puu[c] * Duu / np.trace(Duu) | |||
Duu = Puu[c] * Duu | |||
trace = np.trace(Duu) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not the same as the original even in the normal case. The order of the two lines above would need to be swapped.
In a week or two I expect to be able to have a look at the Pugh-Woodworth book recommended by Richard Ray; I hope it will have a clear explanation of a replacement for the problematic calculation we have now. @DanCodigaMWRA, do you still work with tides? If so, do you have some ideas about this? Or know someone with the expertise and willingness to help out? |
I don't have the time unfortunately to dive into this, sorry about this. I would like to point out the pangeo-pytide library though. |
@efiring thanks for asking. I do still work on tides a bit. Mostly just helping folks who are using the Matlab UTide functions. As for the specific topic here, latitudes b/w -5 and +5, I don't have much insight and would have to dig around to try to come up to speed. The quoted documentation from t-tide shown above seems to attribute it to Foreman. He was helpful to me back when I was putting UTide together, would be worth getting his input if possible. p.s. When working on tides I am @DanCodiga (not @DanCodigaMWRA). p.s.s. I do aspire to revamp UTide as it has been almost 10 years now. I will probably start with a survey of all users to learn what would be the most helpful next step, i.e. update the Matlab version or the python version (or build an R version?); help you folks improve the python version here; add new features; add better tutorial/documentation; etc. (If it's python-focused I would see it as a chance for me to finally get up to speed with using github too...) |
No description provided.