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

MESMER-X: Test distrib_cov #540

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

veni-vidi-vici-dormivi
Copy link
Collaborator

  • Tests added
  • Fully documented, including CHANGELOG.rst

Copy link

codecov bot commented Oct 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 62.71%. Comparing base (72d2fd9) to head (fb8d7c9).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #540      +/-   ##
==========================================
+ Coverage   62.00%   62.71%   +0.71%     
==========================================
  Files          50       50              
  Lines        3561     3554       -7     
==========================================
+ Hits         2208     2229      +21     
+ Misses       1353     1325      -28     
Flag Coverage Δ
unittests 62.71% <100.00%> (+0.71%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@mathause mathause left a comment

Choose a reason for hiding this comment

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

Thanks that looks good.

"SLSQP",
"CG",
"Newton-CG",
if self.method_fit not in [
Copy link
Member

Choose a reason for hiding this comment

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

nice

Comment on lines +619 to +620
" 'threshold_stopping_rule' and 'ind_year_thres', threshold_stopping_rule",
"and 'ind_year_thres' must be used together, and only for 'fcNLL'",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
" 'threshold_stopping_rule' and 'ind_year_thres', threshold_stopping_rule",
"and 'ind_year_thres' must be used together, and only for 'fcNLL'",
" 'threshold_stopping_rule' and 'ind_year_thres', threshold_stopping_rule,"
" and 'ind_year_thres' must be used together, and only for 'fcNLL'",

@@ -649,7 +651,7 @@ def _get_weights_nll(self, n_bins_density=40):
# interpolating over whole region
gmt_hist, edges = np.histogramdd(sample=tmp, bins=bins.T)

gmt_bins_center = [0.5 * (edge[1:] + edges[:-1]) for edge in edges]
gmt_bins_center = [0.5 * (edge[1:] + edge[:-1]) for edge in edges]
Copy link
Member

Choose a reason for hiding this comment

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

nice find

@@ -38,6 +38,7 @@ def wrapper(*args, **kwargs):
return wrapper


# TODO: would want to switch this, have a distrib class that takes xarrays and have a training func that potentially works on xarrays
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# TODO: would want to switch this, have a distrib class that takes xarrays and have a training func that potentially works on xarrays
# TODO: enable distrib class and training func for xarray objs

np.testing.assert_equal(dist.data_pred, {"tas": pred})
np.testing.assert_equal(dist.weights_driver, np.ones(n) / n)
assert dist.n_sample == n
assert dist.expr_fit == expression
Copy link
Member

Choose a reason for hiding this comment

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

Does that actually work? Ah it does but I think it checks that it's the same objects ant thus better to use is to be more explicit.

Suggested change
assert dist.expr_fit == expression
assert dist.expr_fit is expression

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah thanks!

assert dist.boundaries_coeffs == {}
assert dist.first_guess is None
assert dist.func_first_guess is None
assert dist.n_coeffs == 2
Copy link
Member

Choose a reason for hiding this comment

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

I think that should be a property of expr_fit (no need to do that here, though)

data_targ_addtest = rng.normal(loc=2 * pred, scale=0.1, size=n)
data_preds_addtest = {"tas": np.linspace(0, 0.9, n)}
threshold_min_proba = 0.1
boundaries_params = {"loc": [-10, 10], "scale": [0, 1]}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
boundaries_params = {"loc": [-10, 10], "scale": [0, 1]}
boundaries_params = {"loc": [-10, 10], "scale": [-1, 1]}

Could try -1 here to check we end up with 0 again

np.testing.assert_equal(dist.data_targ_addtest, data_targ_addtest)
np.testing.assert_equal(dist.data_preds_addtest, data_preds_addtest)
assert dist.n_sample == n
assert dist.expr_fit == expression
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
assert dist.expr_fit == expression
assert dist.expr_fit is expression

np.array([1, 2, 3]),
{"tas": np.array([1, 2, 3])},
expression,
options_solver="this is not a dictionary",
Copy link
Member

Choose a reason for hiding this comment

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

😄

@veni-vidi-vici-dormivi
Copy link
Collaborator Author

Ahh sorry, this is not at all done yet, I just pushed it to get the coverage report 😅

@veni-vidi-vici-dormivi veni-vidi-vici-dormivi marked this pull request as draft October 4, 2024 07:47
@mathause
Copy link
Member

mathause commented Oct 4, 2024

Ahh sorry, this is not at all done yet, I just pushed it to get the coverage report 😅

Ahh well looks good already 😉

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