Feature/rmax fill nosmooth#114
Feature/rmax fill nosmooth#114SorooshMani-NOAA merged 6 commits intomainoceanmodeling/StormEvents:mainfrom feature/rmax_fill_nosmoothoceanmodeling/StormEvents:feature/rmax_fill_nosmoothCopy head branch name to clipboard
Conversation
|
@WPringle based on your changes the our original regression method did not have smoothing (while Andy's method did?), right? I know we ideally want But my concern is changing the meaning of already implemented method just causes more confusion in the future. I suggest keeping the old no-smooth method name |
|
@SorooshMani-NOAA No, we always had the smoothing. This option is to turn it off, so it will be slightly different to Penny et al (2023) |
|
Oh, yeah sorry, I misread the diff!!! Thanks for the update! Sorry for the confusion |
|
So, when the tests pass I can merge it, thanks |
|
Thanks @SorooshMani-NOAA. I think your comments still have some validity, like could add |
|
Yeah, I agree, it helps clarify things a bit. |
…ult option that does not specify smoothing for some added clarity
| none = None | ||
| persistent = auto() | ||
| regression_penny_2023 = auto() | ||
| regression_penny_2023_with_smoothing = auto() |
There was a problem hiding this comment.
@WPringle you don't need to define a new number, you can just make sure both enum names are the same, e.g.:
In [1]: import enum
In [2]: class E(enum.Enum):
...: a = enum.auto()
...: b = enum.auto()
...: c = b
...: d = enum.auto()
...:
In [3]: E
Out[3]: <enum 'E'>
In [4]: E.a
Out[4]: <E.a: 1>
In [5]: E.b
Out[5]: <E.b: 2>
In [6]: E.c
Out[6]: <E.b: 2>
In [7]: E.d
Out[7]: <E.d: 3>There was a problem hiding this comment.
actually in this case we probably have to set penny_regression_2023 = penny_regression_2023_with_smoothing rather than the other way around, but I think it's still a good idea to keep the resulting enum value the same.
There was a problem hiding this comment.
I was wondering about that, so you can just set c=b as you did in that class ok.
…nding with_smoothing flag
|
@WPringle please let me know when this PR is ready to merge so that we can discuss if any test fails without any apparent reason, thanks! |
|
@SorooshMani-NOAA Yes, it should be ready to merge. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #114 +/- ##
==========================================
+ Coverage 91.71% 91.76% +0.05%
==========================================
Files 19 19
Lines 2087 2100 +13
==========================================
+ Hits 1914 1927 +13
Misses 173 173 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Adding feature
regression_penny_2023_no_smoothingfor using Penny et al (2023) RMW filling without the 24-hr moving mean smoother function.Moving the smoothing to a function for added clarity to the code.