-
Notifications
You must be signed in to change notification settings - Fork 13
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 evaluation type for cams283, tweak rtol fairmode #1329
Conversation
I'd like to discuss this in person at some point @heikoklein |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main-dev #1329 +/- ##
=========================================
Coverage 78.96% 78.97%
=========================================
Files 136 136
Lines 20813 20814 +1
=========================================
+ Hits 16436 16437 +1
Misses 4377 4377
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@@ -77,7 +77,7 @@ def fairmode_stats(obs_var: str, stats: dict) -> dict: | |||
assert np.isclose( | |||
rmsu * beta_mqi, | |||
np.sqrt((bias) ** 2 + (mod_std - obs_std) ** 2 + (2 * obs_std * mod_std * (1 - R))), | |||
rtol=1e-3, | |||
rtol=1e-2, |
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.
This is related to the rounding done for the statistical values.
dc40c69 reduced the accuracy from 1e-10 to 1e-6. Basically, the formula written there is not true for rounded values. I don't know why it is importand for fairmode.
I am not sure what the test here tries to achieve? It compares two different calculations for rms (beta_mqi = rms/rmsu), but the second one with rounded values?
…a type long evaluation that does not span across 2 different years
tested it with a "long" mos run |
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.
Looks good, just some cleanup.
@@ -19,6 +19,7 @@ | |||
|
|||
class EvalType(str, Enum): | |||
LONG = "long" | |||
LONG2 = "long2" |
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.
remove LONG2
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.
thanks I had forgotten this
Change Summary
if the type of evaluation is 'long' but it does not span across more than 1 year, avoid defining periods as years via
make_period_ys
plus a tweak in rtol for fairmode because a test run with the previous vaule failed the check
Related issue number
this is part of tackling https://github.com/metno/AeroToolsIssues/issues/76
and is the code that makes possible https://aeroval-test.met.no/charlien/pages/evaluation/?project=cams2-83&experiment=test-mos-last-seasons-special
Checklist