-
Notifications
You must be signed in to change notification settings - Fork 2
Bugfix/leadtime multilandfall #94
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
Conversation
|
Hi @FariborzDaneshvar-NOAA this includes an old update that was "luckily" on Hercules, so we could run all storms on Hercules, and then also a fix for the issue @WPringle uncovered about landfall order - in specific that impacted Matthew 2016, but still it is worth adding the fix! Please let me know if you see any issues with my changes, I haven't been spending a lot of time on this repo recently. Thanks! |
|
@SorooshMani-NOAA Sorry for the late response. It only gets the first landfall, correct? |
|
There's no rush for merging this ... whenever you get to run a new ensemble, if you can please test this and make sure first landfall is being simulated then I'll merge. We have to later add an input parameter in yaml for selecting landfall, but for now this is just a bugfix! |
|
@SorooshMani-NOAA which storm do you recommend to test? Matthew or Irma? |
|
If you could verify both work fine it'd be great, thanks! |
|
I ran the workflow (without this fix) for Irma 2017, and Matthew 2016. I got this error for getting Matthew hurricane info.: |
|
Can you double check to see if you have ondemand-storm-workflow/stormworkflow/prep/hurricane_data.py Lines 65 to 69 in 630a1e6
ondemand-storm-workflow/stormworkflow/prep/hurricane_data.py Lines 64 to 68 in 1648dd7
|
That's right! I had the wrong one! and it worked after this fix |
|
Here are the dates from
They are the same for both setups. I also checked |
|
@FariborzDaneshvar-NOAA I think Irma has a single landfall in the leadtime file. For Matthew this shouldn't be the case ... let's talk about it. Actually when I get a chance, I probably need to just add a unit test for this |
|
@FariborzDaneshvar-NOAA I added the unit test and a sample leadtime file. Note that for storm C, the order of landfalls are flipped, so it needs sorting. No rush, please at your convenience checkout this test and confirm that removing sort part of the new code causes the test to fail. Thanks! You can run the test by installing |
|
I just noticed the CI test failed ... please wait for the CI test to pass, then try it on your side!! |
|
@FariborzDaneshvar-NOAA you can look check the code now (whenever you have time) |
|
@SorooshMani-NOAA I ran Should I complete the PR? |
|
I then removed
And ran the test again. This time the test failed as expected: |
Fixing a couple of issues related to picking up the leadtime dates for multi-landfall storms