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

Fix for compatibility with ADCIRC GAHM #76

Merged
merged 4 commits into from
Jun 23, 2023

Conversation

WPringle
Copy link
Contributor

The ADCIRC GAHM model and the aswip pre-processor tool requires the forecast_hours to be non-zero in the fort.22 even for a best-track event (i.e., it doesn't read the datetime entries).

Added functionality to output the non-zero forecast hours when the VortexTrack.to_file() function is used with .22 filename suffix and advisory="BEST" or advisory=ATCF_Advisory.BEST is called, e.g.,

from stormevents.nhc import VortexTrack
from stormevents.nhc.atcf import ATCF_Advisory

event = VortexTrack.from_storm_name('ida', 2021)
event.to_file('Ida_besttrack.22', advisory=ATCF_Advisory.BEST, overwrite=True)
event.to_file('Ida_besttrack.22', advisory='BEST', overwrite=True)

This function call is added to one of the tests for writing Florence to file. The corresponding reference file has been updated with the non-zero forecast hour entries.

…visory.BEST advisory type, to make sure it is compatible with ADCIRC aswip/GAHM functionality
…ST. Adding to test output and updating the reference file with the non-zero entries.
@WPringle
Copy link
Contributor Author

@SorooshMani-NOAA Don't understand why tests failing on this too. deleted all output files before running them on my end and all tests passed.

@SorooshMani-NOAA
Copy link
Collaborator

SorooshMani-NOAA commented Apr 21, 2023

@WPringle let me test on my side as well and see what I get.

Update
@WPringle it seems the reason behind many of these failures is the updated version of pandas (https://pandas.pydata.org/docs/whatsnew/v2.0.0.html). To be more specific:

  • Removed deprecated Series.append(), DataFrame.append(), use concat() instead (GH35407)

and

  • Change the default argument of regex for Series.str.replace() from True to False. Additionally, a single character pat with regex=True is now treated as a regular expression instead of a string literal. (GH36695, GH24804)

@SorooshMani-NOAA
Copy link
Collaborator

@WPringle we can either take care of these pandas issues here, or we can first create a separate branch, fix the dependency issues, merge and then merge main into this branch and rerun tests.

@WPringle
Copy link
Contributor Author

@WPringle we can either take care of these pandas issues here, or we can first create a separate branch, fix the dependency issues, merge and then merge main into this branch and rerun tests.

Thanks for finding the problem. Maybe let's make the separate branch and fix these pandas problems first. I am OK either way though.

@SorooshMani-NOAA
Copy link
Collaborator

OK, let me create a new branch on my machine and try to address it

@SorooshMani-NOAA
Copy link
Collaborator

SorooshMani-NOAA commented Apr 21, 2023

@WPringle I noticed another part of the issue is actually the fact that right now the "realtime" (current year) list of storms returns empty and at least on of the tests fail due to that! I will try to find a better test for its replacement in the same branch that I'm fixing pandas dependency

@SorooshMani-NOAA
Copy link
Collaborator

@WPringle #79 fixes the pandas issue. After its merge, you can merge main to your branch and retest

@SorooshMani-NOAA SorooshMani-NOAA self-assigned this Jun 22, 2023
@SorooshMani-NOAA
Copy link
Collaborator

@WPringle is this pull request still relevant? I think I missed merging it before. In any case it runs into the same issue as I had with those two tests failing out of no where! I found out if I keep repeating (rerunning) the test enough they succeed! Please let me know if this should be merged or is no longer relevant. Thanks

@WPringle
Copy link
Contributor Author

@SorooshMani-NOAA We still need the fort.22 to fill in forecast_hours column for the best-track, so yes I think still necessary.

@codecov
Copy link

codecov bot commented Jun 23, 2023

Codecov Report

Merging #76 (ad44fec) into main (197da88) will increase coverage by 0.14%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #76      +/-   ##
==========================================
+ Coverage   91.18%   91.33%   +0.14%     
==========================================
  Files          18       18              
  Lines        1872     1927      +55     
==========================================
+ Hits         1707     1760      +53     
- Misses        165      167       +2     
Impacted Files Coverage Δ
stormevents/stormevent.py 95.69% <ø> (ø)
stormevents/nhc/track.py 92.76% <100.00%> (+0.22%) ⬆️
stormevents/usgs/base.py 100.00% <100.00%> (ø)
tests/test_nhc.py 100.00% <100.00%> (ø)
tests/test_stormevent.py 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

@SorooshMani-NOAA SorooshMani-NOAA merged commit 79f5ab1 into main Jun 23, 2023
11 checks passed
@SorooshMani-NOAA SorooshMani-NOAA deleted the bug_fix/fort22_gahm_fix branch June 23, 2023 14:24
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.

None yet

2 participants