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

Add option for choosing rmw fill method #112

Merged
merged 6 commits into from
Aug 22, 2024
Merged

Conversation

SorooshMani-NOAA
Copy link
Collaborator

Defaults to PSurge v2.9 method.

@SorooshMani-NOAA
Copy link
Collaborator Author

@WPringle I'm not sure if it'd be helpful or not to accept str type as input or we should leave it to the user to import the RMWFillMethod and pass the write enum. Otherwise this seems to be working fine.

Also I was wondering what you think about the names none, persistent and psurge_v2_9?

I defaulted it to be the new psurge method.

Please let me know what you think ... I'll add some tests soon

Copy link

codecov bot commented Aug 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.71%. Comparing base (0ce50e6) to head (b65278c).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #112      +/-   ##
==========================================
+ Coverage   91.38%   91.71%   +0.32%     
==========================================
  Files          19       19              
  Lines        2009     2087      +78     
==========================================
+ Hits         1836     1914      +78     
  Misses        173      173              

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


🚨 Try these New Features:

@WPringle
Copy link
Contributor

@SorooshMani-NOAA Thank you! The psurge_v2_9 naming is the one I'm unsure about as it is not clear at first what it means. On the other hand it is specific, which is good. Could do: regression_psurge2_9 or something like that?

@SorooshMani-NOAA
Copy link
Collaborator Author

@WPringle I also would like it to be short, so maybe penny_etal_2023? Let's bring it up in the psurge dev meeting

@SorooshMani-NOAA
Copy link
Collaborator Author

Other name suggestions discussed:

  • penny_2023
  • rmw_regression
  • regression_penny_etal_2023

I agree that having regression in the name says what is happening, like persistent says persistent but, then penny_2023 is much shorter and more exact. I'll give it more thought and if I didn't get any more feedback (either in the internal chat or here) I'll probably go with penny_2023.

By the way @WPringle having only regression could also mean future "regression" methods need to be named regression_2, _3, etc. So having paper ref is much better, but also I think having both "regression" and paper ref in the name is just too long to type! 😆

@SorooshMani-NOAA SorooshMani-NOAA linked an issue Aug 22, 2024 that may be closed by this pull request
@SorooshMani-NOAA SorooshMani-NOAA merged commit bcb1948 into main Aug 22, 2024
11 checks passed
@SorooshMani-NOAA SorooshMani-NOAA deleted the feature/rmw_option branch August 22, 2024 19:42
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.

Provide options for filling in forecast values
2 participants