fixes for 'lat=0' and 0 time series issues#62
fixes for 'lat=0' and 0 time series issues#62apatlpo wants to merge 4 commits intowesleybowman:masterfrom
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.
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: |
utide/confidence.py
Outdated
|
|
||
| if not opt.white: | ||
| Duu = Puu[c] * Duu / np.trace(Duu) | ||
| Duu = Puu[c] * Duu |
There was a problem hiding this comment.
There is a trailing space after Duu.
utide/confidence.py
Outdated
| 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.
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.
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. |
efiring
left a comment
There was a problem hiding this comment.
- 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
rrvalues from 5N to 5S. But I don't understand the math and physics behindrrright now, so I don't know how to handle it.
| 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.
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.