noaa-ocs-modeling / EnsemblePerturbation

perturbation of coupled model input over a space of input variables
https://ensembleperturbation.readthedocs.io
Creative Commons Zero v1.0 Universal
7 stars 3 forks source link

Enhance/gahm converge #159

Closed WPringle closed 3 weeks ago

WPringle commented 1 month ago

In some instances, the code for computing the isotach velocity Vr using the GAHM equation could not converge with the original algorithm.

Changes have been made to add a correction rate when calculating the updated alpha as iterations become large.

https://github.com/oceanmodeling/ondemand-storm-workflow/issues/75#issuecomment-2321137953

codecov-commenter commented 1 month ago

Codecov Report

Attention: Patch coverage is 97.56098% with 1 line in your changes missing coverage. Please review.

Project coverage is 25.56%. Comparing base (a2e545b) to head (d3b10cf).

Files with missing lines Patch % Lines
ensembleperturbation/perturbation/atcf.py 95.65% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #159 +/- ## ========================================== + Coverage 20.94% 25.56% +4.62% ========================================== Files 28 29 +1 Lines 3987 4017 +30 ========================================== + Hits 835 1027 +192 + Misses 3152 2990 -162 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

SorooshMani-NOAA commented 1 month ago

@WPringle are the changes finalized or would you like to add more stuff to this PR, e.g. convergence test cases, etc.?

WPringle commented 1 month ago

@SorooshMani-NOAA The convergence test case is a good potential idea! But I was thinking it is finalized..

SorooshMani-NOAA commented 1 month ago

As we discussed in the meeting, we'll test this update on Laura, Irma and Florence (just the setup part) and if all is good we'll merge the PR. I can also add my own test code to assure convergence in ci tests.

SorooshMani-NOAA commented 1 month ago

@WPringle I added a test with timeout failure and now I see that Laura 2020 is failing. Can you please spend some more time on it?

See: https://github.com/noaa-ocs-modeling/EnsemblePerturbation/blob/enhance/gahm_converge/tests/test_perturb.py

From your env (including newly added pytest-timeout) you should be able to run:

pytest -k perturb_perf tests/

to see the failure with all of the latest updates. It doesn't actually time out, because your max iter check fails before that. For the older code (before this branch) this test times out on Irma (as expected).

Please let me know what you think

@FariborzDaneshvar-NOAA I'm already running the workflow with this update on PW. But give this test I think I can already skip that!

FariborzDaneshvar-NOAA commented 1 month ago

@FariborzDaneshvar-NOAA I'm already running the workflow with this update on PW. But give this test I think I can already skip that! @SorooshMani-NOAA I'm not sure if I understood your comment correctly. Is there anything I need to do with this regard?

SorooshMani-NOAA commented 1 month ago

@FariborzDaneshvar-NOAA I just meant that I'm running the workflow with the convergence update for testing. So you don't need to worry about it.

SorooshMani-NOAA commented 1 month ago

@WPringle see https://github.com/noaa-ocs-modeling/EnsemblePerturbation/actions/runs/10724638600. You can get the latest (your+mine) from the same branch as before: https://github.com/noaa-ocs-modeling/EnsemblePerturbation/tree/enhance/gahm_converge

WPringle commented 1 month ago

@SorooshMani-NOAA thanks man, I will work on it, I have an idea

SorooshMani-NOAA commented 1 month ago

@WPringle just an additional note: I tried running the workflow with these updates for 29 (instead of 39) perturbations and it works for 29 for all storms, however in the test code it fails to converge, which is strange!! If I try 29 ensembles (or even 30) for the test code, it still fails! I don't know how the full workflow went through! During my tests with the gahm convergence updates!

WPringle commented 1 month ago

@SorooshMani-NOAA The tests should pass now, I implemented a real bisection method. Update: The convergence works but the results are different. Let me check them for accuracy.

SorooshMani-NOAA commented 1 month ago

@WPringle thanks for the update. .. it seems that now some of the earlier (file-based) tests fail can you check and see if the difference in results is sensible or not, if it is then we can update the test refs and merge.

WPringle commented 1 month ago

@SorooshMani-NOAA Yes, doing that now, mostly seems reasonable, very minor adjustment by 1 n mi for a single entry in most cases. but will double check.

WPringle commented 1 month ago

@SorooshMani-NOAA I think it is good to go now.

There is one last thing that is interesting to note: Sometimes the isotach radii can be less than the forecasted RMW. In that case I preserve that restriction and only search for the isotach radii < RMW rather than > RMW. It is possible also to make a restriction on isotach radii to always be >RMW, which would result in modifying the original isotach radii in the zero perturbation track. But for now, I think what we have is good as the regressed forecasted RMW may not be as accurate as the original forecasted isotach radii.