remove use_theorycovmat key #2216
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
scarlehoff
reviewed
Nov 18, 2024
| @@ -538,14 +538,11 @@ def procs_corrmat(procs_covmat): | |||
| def results(dataset: DataSetSpec, pdf: PDF, covariance_matrix, sqrt_covmat): | |||
| """Tuple of data and theory results for a single pdf. The data will have an associated | |||
| covariance matrix, which can include a contribution from the theory covariance matrix which | |||
| is constructed from scale variation. The inclusion of this covariance matrix by default is used | |||
| where available, however this behaviour can be modified with the flag `use_theorycovmat`. | |||
| is constructed from scale variation. | |||
There was a problem hiding this comment.
So this docstring was not true?
But the part of The inclusion of this covariance matrix by default is used is still true isn't it?
There was a problem hiding this comment.
I didn't trace back how covariance_matrix and sqrt_covmat are produced and whether they include the theory covmat or not, but at least the comment about use_theorycovmat was wrong
|
I did want to merge that, so thanks for that - I had just forgotten I also wanted to do this. But doesn't really matter that it has it's own PR Good call, I limited my search only to .py files... |
scarlehoff
approved these changes
Nov 18, 2024
RoyStegeman
force-pushed
the
update_th_covmat
branch
2 times, most recently
from
6565a7a to
85608b6
Compare
This key was only used in a check, where it is forced to be set to true, though an error is raised if this is not the case, so it serves little purpose. Probably a remnant of a time where it could be used to toggle the use of the theory covmat.
RoyStegeman
force-pushed
the
update_th_covmat
branch
from
85608b6 to
b4c1d07
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I was adding this to #2198 but when I tried to commit I saw that had been merged - I'd still like to add it though
This key was only used in a check, where it is forced to be set to true, though an error is raised if this is not the case, so it serves little purpose. Probably a remnant of a time where it could be used to toggle the use of the theory covmat.