-
Notifications
You must be signed in to change notification settings - Fork 14
covmat-by-reference-bug #2463
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
covmat-by-reference-bug #2463
Changes from all commits
47f1856
434d573
80e6b81
472d92c
74cf8ed
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,7 +9,15 @@ | |
|
|
||
| from nnpdf_data.commondataparser import load_commondata | ||
| from validphys.api import API | ||
| from validphys.covmats import dataset_t0_predictions, reorder_thcovmat_as_expcovmat, sqrt_covmat | ||
| from validphys.covmats import ( | ||
| dataset_inputs_t0_total_covmat, | ||
| dataset_inputs_t0_total_covmat_separate, | ||
| dataset_inputs_total_covmat, | ||
| dataset_inputs_total_covmat_separate, | ||
| dataset_t0_predictions, | ||
| reorder_thcovmat_as_expcovmat, | ||
| sqrt_covmat, | ||
| ) | ||
| from validphys.tests.conftest import DATA, HESSIAN_PDF, PDF, THEORYID | ||
|
|
||
| # Experiments which have non trivial correlations between their datasets | ||
|
|
@@ -153,3 +161,24 @@ def test_single_datapoint(single_data_single_point_internal_cuts_config): | |
| # Ensure the dataset is only a single datapoint | ||
| assert ld.ndata == 1 | ||
| ld.systematic_errors(t0_predictions) | ||
|
|
||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The test is fine per se, but I'd like to hear @scarlehoff's opinion. In my view, the test prevents us from repeating the same mistake, although now that we are aware of the issue it might seem a bit superfluous. Still, I think we should keep it as a reminder for the future and as a signal for new members.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Many of our tests are built on the blood of our previous mistakes. I'm happy with superflous tests. |
||
| @pytest.mark.parametrize( | ||
| "combine_fn", | ||
| [ | ||
| dataset_inputs_t0_total_covmat_separate, | ||
| dataset_inputs_total_covmat_separate, | ||
| dataset_inputs_t0_total_covmat, | ||
| dataset_inputs_total_covmat, | ||
| ], | ||
| ) | ||
| def test_covmat_summing_helpers_do_not_mutate_inputs(combine_fn): | ||
| exp = np.array([[1.0, 0.2], [0.2, 2.0]]) | ||
| th = np.array([[0.5, 0.1], [0.1, 0.3]]) | ||
| exp_before = exp.copy() | ||
|
|
||
| result = combine_fn(exp, th) | ||
|
|
||
| np.testing.assert_allclose(exp, exp_before) | ||
| np.testing.assert_allclose(result, exp_before + th) | ||
| assert result is not exp | ||
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.
If I was both clever and handsome I would be too powerful for this world.
Anyway, this can be merged I guess.
(I've done this change because reading https://github.com/conda-incubator/setup-miniconda#important I thought macos, which has its own flavour of bash due to licence issues, might be messing with the environment activation, but I don't fully know why this fixes it other than "apple bad")