-
Notifications
You must be signed in to change notification settings - Fork 13
Dispatching Python covariance matrices #1477
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
Conversation
|
I've rebased your commit on top of #1430 to see whether the test passes also there https://github.com/NNPDF/nnpdf/runs/4333530141?check_suite_focus=true |
|
Looks like the linux tests are passing on the rebase |
|
|
||
|
|
||
| @check_dataset_cuts_match_theorycovmat | ||
| def covmat( |
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.
Where has all this code and documentation gone? How are the norm_threshold and use_weights_in_covmat arguments handled?
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.
The weights are used in the lower level functions
nnpdf/validphys2/src/validphys/covmats.py
Line 35 in eeae19b
| use_weights_in_covmat=True, |
|
I am a bit worried that this is going to obscure |
|
It would be good to have tests that cover the various options passed to the covmat. E.g. the thing with the |
Yeah I couldn't figure out how to do this without having a dummy function which the
will add now |
|
One thing I'd like to do someday is have a more restricted version of which does the same thing but additionally knows how to follow the possibilities and produce help. |
6521c72 to
cb092fe
Compare
|
Yes that would be nice, since it could then dispatch another |
|
Should I / Can I rebase #1430 on top of this one already or are more changes needed? |
|
I’m done with it if @Zaharid is happy
On 29 Nov 2021, at 10:14, Juan M. Cruz-Martinez ***@***.***> wrote:
Should I / Can I rebase #1430<#1430> on top of this one already or are more changes needed?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub<#1477 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AKMAJEG3OFDACMHS4FZTF4TUONHAJANCNFSM5I2PCJUQ>.
|
|
It seems ok. I would hope that the documentation challenges are addressed by #1478. |
|
Shall we merge this? |
scarlehoff
left a comment
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'm in favour of merging since everything seems to work in this one and also down the line (#1430 )
We now dispatch covmats purely from python. Addresses various points including #1476 (comment) and #1475 (comment)
Let's see what the tests do