search

oceanmodeling / StormEvents

Python interfaces for observational data surrounding named storm events, born from @jreniel's ADCIRCpy
https://stormevents.readthedocs.io
GNU General Public License v3.0
24 stars 8 forks source link

Add option for choosing rmw fill method #112

Closed SorooshMani-NOAA closed 2 months ago

SorooshMani-NOAA commented 2 months ago

Defaults to PSurge v2.9 method.

SorooshMani-NOAA commented 2 months ago

@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

codecov[bot] commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

WPringle commented 2 months ago

@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 commented 2 months ago

@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 commented 2 months ago

Other name suggestions discussed:

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! 😆