-
Notifications
You must be signed in to change notification settings - Fork 3
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
Enhance/gahm converge #159
Enhance/gahm converge #159
Conversation
… of iterations needed and adding RunTimeError when iterations exceed 1000
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ 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 ☔ View full report in Codecov by Sentry. |
@WPringle are the changes finalized or would you like to add more stuff to this PR, e.g. convergence test cases, etc.? |
@SorooshMani-NOAA The convergence test case is a good potential idea! But I was thinking it is finalized.. |
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. |
@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? From your env (including newly added 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 I just meant that I'm running the workflow with the convergence update for testing. So you don't need to worry about it. |
@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 |
@SorooshMani-NOAA thanks man, I will work on it, I have an idea |
@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! |
…previously I saw over 50 iterations at times, now I see mostly 15 iterations needed and up to no more than 30 with the tests performed
@SorooshMani-NOAA The tests should pass now, I implemented a real bisection method. |
@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. |
@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. |
…is converged. setting alpha_hi to always be 1 at the beginning
@SorooshMani-NOAA I think it is good to go now. There is one last thing that is interesting to note: |
@@ -58,11 +58,11 @@ AL, 06, 2018091518, , BEST, 0, 336N, 798W, 47, 999, TS, 34, NEQ, 136, | |||
AL, 06, 2018091600, , BEST, 0, 336N, 802W, 43, 999, TS, 34, NEQ, 128, 127, 148, 152, 1013, 240, 110, 50, 0, L, 0, , 270, 2, FLORENCE, 22 | |||
AL, 06, 2018091606, , BEST, 0, 336N, 808W, 38, 1000, TS, 34, NEQ, 126, 124, 0, 0, 1013, 260, 110, 40, 0, L, 0, , 270, 3, FLORENCE, 23 | |||
AL, 06, 2018091612, , BEST, 0, 336N, 815W, 33, 1003, TS, 34, NEQ, 0, 0, 0, 0, 1013, 280, 140, 40, 0, L, 0, , 270, 3, FLORENCE, 24 | |||
AL, 06, 2018091618, , BEST, 0, 341N, 821W, 28, 1007, TD, 0, , 0, 0, 0, 0, 1013, 300, 140, 40, 0, L, 0, , 315, 4, FLORENCE, 25 |
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.
@WPringle thanks for checking, just to double check, can you explain why these values changes from 0
to some large values? Is the new approach avoiding some nan
values that we previously used to get?
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.
@SorooshMani-NOAA Yes, the old approach didn't actually converge in some cases so would just get a 0. Now it should always converge to something if it is possible to.
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.oceanmodeling/ondemand-storm-workflow#75 (comment)