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

Bugfix/gahm converge3 #166

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

WPringle
Copy link
Contributor

@WPringle WPringle commented Nov 5, 2024

Fix for convergence issue regarding #165

@SorooshMani-NOAA SorooshMani-NOAA linked an issue Nov 6, 2024 that may be closed by this pull request
@SorooshMani-NOAA SorooshMani-NOAA self-assigned this Nov 6, 2024
@WPringle
Copy link
Contributor Author

WPringle commented Nov 6, 2024

I'm still wondering about if should limit Rrat to be always < 1 (0.95) or to allow it to be greater than 1 also. The issue is sometimes the provided 64-kt isotachs of (mainly) western quadrants are < Rmax. In our assumption this doesn't make sense but it can be real because the Rmax for that quadrant may be smaller than the overall Rmax.
Let me rethink the theory.

@SorooshMani-NOAA
Copy link
Collaborator

@WPringle I'm not sure if I fully understand; when you say:

Rmax for that quadrant may be smaller than the overall Rmax

Do you mean with a given ATCF row (as defined in ATCF forecast) where RAD is equal to 64, the RAD3 and RAD4 columns could be smaller than RMW column?

What is the problem then with going with the abs value difference like before instead of limiting to be less than 0.95? is 1.05 going to cause the same convergence issue? I thought we only want to avoid 1 ratio, right?

@WPringle
Copy link
Contributor Author

WPringle commented Nov 7, 2024

@SorooshMani-NOAA Yeah originally, the idea was just to avoid close to one which gives instability problem. We can also do the 1.05 limit as well. But when I compare the test output files there are some cases that < 0.95 seems more reasonable, may other cases allowing > 1.05 more reasonable. So just want to think more about assumptions and how to edit for consistency.

@SorooshMani-NOAA
Copy link
Collaborator

@WPringle may I ask what is your criteria when saying

seems more reasonable

? Is it comparing it against results from the current main branch approach? Or something else? Like looking at the full distribution and calculating something on it?

@WPringle
Copy link
Contributor Author

WPringle commented Nov 7, 2024

@SorooshMani-NOAA Basically yeah how the perturbation values change from the current main branch approach, and thinking about what makes more sense from a physical perspective. It is perhaps a somewhat subjective judgement I admit.

@WPringle
Copy link
Contributor Author

WPringle commented Nov 11, 2024

@SorooshMani-NOAA I decided that it makes sense to limit Rrat <= 1, and we can do that through a Rankine vortex assumption, that is, V = Vmax*RMW/r for r > RMW.
therefore Rrat = RMW / R64 = Vr(R64) / Vmax

…by the GAHM code in ADCIRC/PAHM/SCHISM. A catch for 0-kt isotach to avoid trying to fit an isotach radius to it
@WPringle
Copy link
Contributor Author

@SorooshMani-NOAA Some changes based on looking into the GAHM code and literature further. Overall, the effects are mostly minimal, with changes usually about 1 n mi to isotach radii here and there.

@SorooshMani-NOAA
Copy link
Collaborator

@WPringle if there's no rush, I will merge this when I get back from HFIP

…e isotach_radii in each quadrant. i.e., the rotation will be angled towards the quadrant with the largest isotach radius
@WPringle
Copy link
Contributor Author

WPringle commented Nov 13, 2024

Latest change makes the isotach radii more consistent with those provided by NHC. For example if the only non-zero isotach radii value is in the SEQ then the update keeps it to the SEQ. Previously, due to assumed rotation of translation speed towards the NEQ, the isotach radii would shift from the SEQ to the NEQ in this case.

@WPringle
Copy link
Contributor Author

@SorooshMani-NOAA Not sure why failing as the test files were updated and ran on my side fine..

@SorooshMani-NOAA
Copy link
Collaborator

@WPringle let me run it locally on my side as well to see what happens

@SorooshMani-NOAA
Copy link
Collaborator

SorooshMani-NOAA commented Nov 15, 2024

@WPringle I see the same change in my local.. I'm updating the test reference here: 1403d99. Feel free to push this to your branch if all looks good! Then I can merge to main

@WPringle
Copy link
Contributor Author

@WPringle I see the same change in my local.. I'm updating the test reference here: 1403d99. Feel free to push this to your branch if all looks good! Then I can merge to main

Interesting thanks! I'll take another look on my local and see if can work it out.

@SorooshMani-NOAA
Copy link
Collaborator

@WPringle are you running your tests on a mac? Maybe there's a difference between results in mac vs linux that is causing the issue!

@WPringle
Copy link
Contributor Author

@SorooshMani-NOAA No it's linux. I think it is a tricky precision issue based on package versions etc. Looking further so we could avoid the problem.

@SorooshMani-NOAA
Copy link
Collaborator

@WPringle I remember in one of the other packages we had something like this about the precision caused by numpy version ... I'm not sure what we did there ... but I guess I pinned the version of numpy ... which is a short term solution!

@WPringle
Copy link
Contributor Author

@SorooshMani-NOAA So what version of numpy you have?

@SorooshMani-NOAA
Copy link
Collaborator

$ pip list | grep "\(numpy\|scipy\)"
numpy                1.26.4
scipy                1.12.0

…otach_radius when it is given as zero, based on the ratio of the other isotach_radii so as to avoid guessing B from traditional Holland model
@WPringle
Copy link
Contributor Author

$ pip list | grep "\(numpy\|scipy\)"
numpy                1.26.4
scipy                1.12.0

Thanks, I still get different result with the same versions of these packages. What other packages could be important I wonder.

@SorooshMani-NOAA
Copy link
Collaborator

@WPringle I'm not sure, maybe python version and underlying numpy c libraries(?!). btw do you delete your data/output every time before running?

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

Successfully merging this pull request may close these issues.

GAHM parameter convergence issue
2 participants