Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

using the Psurge v2.9 regression method for filling in Rmax in OFCL advisories#96

Merged
SorooshMani-NOAA merged 21 commits intomainoceanmodeling/StormEvents:mainfrom
feature/rmax_Psurgev29oceanmodeling/StormEvents:feature/rmax_Psurgev29Copy head branch name to clipboard
Apr 24, 2024
Merged

using the Psurge v2.9 regression method for filling in Rmax in OFCL advisories#96
SorooshMani-NOAA merged 21 commits intomainoceanmodeling/StormEvents:mainfrom
feature/rmax_Psurgev29oceanmodeling/StormEvents:feature/rmax_Psurgev29Copy head branch name to clipboard

Conversation

@WPringle
Copy link
Copy Markdown
Contributor

@WPringle WPringle commented Mar 27, 2024

@WPringle
Copy link
Copy Markdown
Contributor Author

WPringle commented Apr 5, 2024

Likely fixed #97 by just sorting the individual advisories by index instead of by "forecast_hours".

@WPringle WPringle marked this pull request as ready for review April 18, 2024 16:39
@SorooshMani-NOAA SorooshMani-NOAA self-assigned this Apr 18, 2024
Comment thread stormevents/nhc/track.py
Comment thread stormevents/nhc/track.py Outdated

# Regression coefficients for the Rmax forecast
# ref: Penny et al. (2023). https://doi.org/10.1175/WAF-D-22-0209.1
def get_regression_coefs(fcst_hr, radii_values):
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@WPringle it'd be nice if you could add a simple test for this function, by just providing fcst hour and radii, what do you expect to see. Just a couple of basic cases.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added in latest commit

Comment thread stormevents/nhc/track.py Outdated
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.70%. Comparing base (79f5ab1) to head (45e3875).
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #96      +/-   ##
==========================================
+ Coverage   91.33%   91.70%   +0.36%     
==========================================
  Files          18       19       +1     
  Lines        1927     1988      +61     
==========================================
+ Hits         1760     1823      +63     
+ Misses        167      165       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@SorooshMani-NOAA

This comment was marked as outdated.

@SorooshMani-NOAA
Copy link
Copy Markdown
Collaborator

@WPringle please let me know when this is ready to merge. All looks good from my perspective (after including comments I made before)

@WPringle
Copy link
Copy Markdown
Contributor Author

@SorooshMani-NOAA ready to merge but an unrelated test is now failing: test_storm_event_coops_product_within_region

@SorooshMani-NOAA
Copy link
Copy Markdown
Collaborator

@WPringle thanks, I'll look into it

@SorooshMani-NOAA
Copy link
Copy Markdown
Collaborator

@WPringle after rerunning the test (to resolve the temporary server issue) it seems that the tests are not updated after the 50kt radius fix. Can you run the tests locally and see if it's doing the right thing for 50kt and 34kt and if so update the test refs? When I run locally I see that some 34kt and 50kt columns are different between the test output and ref.

@WPringle
Copy link
Copy Markdown
Contributor Author

@SorooshMani-NOAA Thanks, yes some of the RMW forecasts did change and I think correctly. updated the reference files.

@SorooshMani-NOAA SorooshMani-NOAA merged commit ee51dd1 into main Apr 24, 2024
@SorooshMani-NOAA SorooshMani-NOAA deleted the feature/rmax_Psurgev29 branch April 24, 2024 17:15
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.

2 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.