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

Feature/rmax fill nosmooth #114

Merged
merged 6 commits into from
Jan 27, 2025
Merged

Conversation

WPringle
Copy link
Contributor

Adding feature regression_penny_2023_no_smoothing for 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.

@SorooshMani-NOAA
Copy link
Collaborator

@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 regression_penny_2023 to be exactly equal to what Andy's paper had and we also want to try the version without smoothing (which is what we actually did)

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 regeression_penny_2023, and let regression_penny_2023_no_smoothing also be equal to that, and then also add a third method regression_penny_2023_with_smoothing. This way for regression_penny_2023 we can send deprecation warnings, and the user should basically choose between regression_penny_2023_with_smoothing vs regression_penny_2023_no_smoothing.

@WPringle
Copy link
Contributor Author

@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)

@SorooshMani-NOAA
Copy link
Collaborator

Oh, yeah sorry, I misread the diff!!! Thanks for the update! Sorry for the confusion

@SorooshMani-NOAA
Copy link
Collaborator

So, when the tests pass I can merge it, thanks

@WPringle
Copy link
Contributor Author

Thanks @SorooshMani-NOAA. I think your comments still have some validity, like could add regression_penny_2023_with_smoothing as an option for clarity, that is equal to the one without specifying smoothing. What do you think?

@SorooshMani-NOAA
Copy link
Collaborator

Yeah, I agree, it helps clarify things a bit.

…ult option that does not specify smoothing for some added clarity
@@ -8,6 +8,7 @@ class RMWFillMethod(Enum):
none = None
persistent = auto()
regression_penny_2023 = auto()
regression_penny_2023_with_smoothing = auto()
Copy link
Collaborator

Choose a reason for hiding this comment

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

@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>

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was wondering about that, so you can just set c=b as you did in that class ok.

@SorooshMani-NOAA
Copy link
Collaborator

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

@WPringle
Copy link
Contributor Author

@SorooshMani-NOAA Yes, it should be ready to merge.

@SorooshMani-NOAA SorooshMani-NOAA self-assigned this Jan 27, 2025
Copy link

codecov bot commented Jan 27, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.76%. Comparing base (bcb1948) to head (c6ca722).
Report is 1 commits behind head on main.

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.
📢 Have feedback on the report? Share it here.

@SorooshMani-NOAA SorooshMani-NOAA merged commit e4a3aab into main Jan 27, 2025
10 checks passed
@SorooshMani-NOAA SorooshMani-NOAA deleted the feature/rmax_fill_nosmooth branch January 27, 2025 20:46
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