-
Notifications
You must be signed in to change notification settings - Fork 55
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
adding RSS and RMS #176
adding RSS and RMS #176
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #176 +/- ##
==========================================
+ Coverage 90.74% 90.77% +0.02%
==========================================
Files 23 23
Lines 2475 2482 +7
==========================================
+ Hits 2246 2253 +7
Misses 229 229 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi,
Thanks a lot for your contribution!
I have a minor comment that needs clarification.
float | ||
Root Mean Square of the residuals. | ||
""" | ||
return np.sqrt(np.nanmean(np.square(self.model_residuals))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry if I'm blind right now, but is this not the same as Variogram.rmse
?
We could update the the existing rmse with this implementation, which is way cleaner; or let Variogram.rmse
just call your function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like mean square of the residuals (RMS) could be the Root Mean Square Error (RMSE), however in the book 1 its not explicitaly menctioned ~ which is why I added it separately; similarly here is used with the same nomenclature 2.
It could be a typo in the book (?) and its just the RMSE, for me leaving RMSE is fine..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pointed my old Variogram.rmse
to your Variogram.root_mean_square
and all the tests still work, so in fact, it is the same thing.
Then, we keep both with Variogram.rmse
simply calling Variogram.root_mean_square
, as it has the much cleaner implementation.
def rss(self): | ||
return self.residual_sum_of_squares |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this, cause I like to have both, the full name and the short name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds great
Thanks again. Just published as |
Contributions:
included and tested the metrics RSS and RSM, it is suggested in the reference below
Reference:
Oliver, M. A., & Webster, R. (2015). Basic steps in geostatistics: the variogram and kriging (No. 11599). Cham, Switzerland: Springer International Publishing.
Link: https://link.springer.com/book/10.1007/978-3-319-15865-5