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

Differences in modelled FES elevations between pyTMD=2.1.4 and pyTMD==2.1.5 #335

Closed
robbibt opened this issue Sep 8, 2024 · 3 comments · Fixed by #336
Closed

Differences in modelled FES elevations between pyTMD=2.1.4 and pyTMD==2.1.5 #335

robbibt opened this issue Sep 8, 2024 · 3 comments · Fixed by #336
Assignees
Labels

Comments

@robbibt
Copy link
Contributor

robbibt commented Sep 8, 2024

I recently noticed that some of our FES2014 integration tests are failing, and realised that pyTMD is producing different results in 2.1.5 vs 2.1.4, sometimes by over 10 cm:

!pip install pyTMD==2.1.4 --quiet

from pyTMD.compute import tide_elevations
import pandas as pd
import numpy as np

tide_elevations(
    x=[122.269294],
    y=[-18.057430],
    delta_time=pd.date_range("2022", "2023", periods=5),
    DIRECTORY="/gdata1/data/tide_models_clipped/",
    MODEL="FES2014",
    EPSG=4326,
    TIME="datetime",
    EXTRAPOLATE=True,
    CUTOFF=np.inf,
)

>>> [1.6378203716778157, 1.4584970325019366, -2.682039596186077, 3.4367875303700806, -0.07229756252779537]
!pip install pyTMD==2.1.5 --quiet

from pyTMD.compute import tide_elevations
import pandas as pd
import numpy as np

tide_elevations(
    x=[122.269294],
    y=[-18.057430],
    delta_time=pd.date_range("2022", "2023", periods=5),
    DIRECTORY="/gdata1/data/tide_models_clipped/",
    MODEL="FES2014",
    EPSG=4326,
    TIME="datetime",
    EXTRAPOLATE=True,
    CUTOFF=np.inf,
)

>>> [1.627442336610703, 1.545339524166975, -2.744346618968223, 3.3916417087573367, 0.03555559012406023]

Longer comparison scatterplot:
image

I'm not yet sure if the 2.1.5 results are better or worse - just that they are different. I haven't tested this exhaustively across all the models, but so far FES models are the only ones that seem different (EOT20 and TPXO9 seem unchanged).

@tsutterley
Copy link
Owner

That's not good. I'll try to track it down.

@tsutterley
Copy link
Owner

tsutterley commented Sep 9, 2024

Hey @robbibt,
A few separate things happening, all of which are related to when I updated arguments.py to include (a lot) more constituents. Some of these changes are permanent as they're fixes to old bugs. Some are new bugs that will be fixed in #336.

  1. The Doodson arguments for eta2 have been corrected. This only affected when inferring from minor constituents.
  2. I am inferring L2b when L2 is known. In 2.1.4, I didn't infer this constituent if L2 was available.
  3. I didn't pass a keyword argument for the recursive calculation of compound tides. So the arguments were the default ones, and not the arguments for FES models. This affected several constituents, and should be fixed in the PR.
  4. I was overwriting the argument for J1 to use the default and not the one for FES models. This should be fixed with the PR.

Thanks for pointing out the problem. I don't think I would have caught it without the nudge. I have a few things I need to check, but I think with the bug fixes I'll put out a release sooner rather than later.

@tsutterley tsutterley self-assigned this Sep 10, 2024
@tsutterley tsutterley added the bug label Sep 10, 2024
@tsutterley tsutterley linked a pull request Sep 10, 2024 that will close this issue
@robbibt
Copy link
Contributor Author

robbibt commented Sep 10, 2024

Fantastic, thanks @tsutterley - this wasn't a problem for us, just unexpected so I thought it was worth raising. Glad to hear it helped clear out some hidden bugs!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants