Return all members when using MC PDFs#1522
Conversation
scarlehoff
force-pushed
the
mc_pdf_to_include_all
branch
from
21e4fea to
0321658
Compare
scarlehoff
force-pushed
the
mc_pdf_to_include_all
branch
from
80b8d4c to
c5425ff
Compare
scarlehoff
added
run-fit-bot
|
Greetings from your nice fit 🤖 !
Check the report carefully, and please buy me a ☕ , or better, a GPU 😉! |
|
almost there |
scarlehoff
added
run-fit-bot
|
This should be ready for review. We can talk this afternoon about who is going to review this because I guess all three PR should be done by the same person and hopefully at the same time. I've run all the examples in the validphys folder with the current commit. I can gzip them all and send them to the reviewer if they wish. The fit bot can be run when we get the extra space. |
Zaharid
left a comment
Zaharid
left a comment
There was a problem hiding this comment.
Will take me a while to go over the bits.
Co-authored-by: Zaharid <zk261@cam.ac.uk>
|
TODO:
As discussed, moving here the minor comments of the other PRs so that they can be merged |
|
@scarlehoff do you want to merge the other PR as they are and keep working here, or close the others and only merge this one? |
Maybe merging them one by one makes doing the "undo" easier in case something is wrong obviously with any of them. But I don't think it makes a difference tbh |
|
TBH I find looking at the diff here a bit confusing, so if we could merge the others and rebase possible further changes here, I'd be happier. |
|
if you approve the others I'll merge them |
|
I've implemented now the changes that were requested on the other two PRs. |
RoyStegeman
left a comment
RoyStegeman
left a comment
There was a problem hiding this comment.
This seems alright. The diffs are a real mess though..
Co-authored-by: Roy Stegeman <roystegeman@live.nl>
You are going to hate me so much when you're made to review #1529... |
|
I think I addressed everything. |
|
@Zaharid are you happy merging this? (it's currently blocked by you) |
| alldata, central, npoints = abs_chi2_data | ||
| cv = float(alldata.central_value()) | ||
| return (np.sqrt((cv - central) / npoints), npoints) | ||
| return (np.sqrt((alldata.error_members().mean() - central) / npoints), npoints) |
There was a problem hiding this comment.
Maybe we should make this and abs_chi2_data Stats as well, but that is another PR.
Zaharid
left a comment
Zaharid
left a comment
There was a problem hiding this comment.
If Roy is happy I am happy.
Co-authored-by: Zaharid <zk261@cam.ac.uk>
3 participants
As the title suggest.
After the two previous PR (#1515 and #1517) where
Statswhere used everywhere, the only two things that are problematic are:Nothing is evidently broken, but many thing might be broken in not obvious ways. The first commit is just so that the regression for the sumrules passes so I can update it in the next commit (where all members will be computed also there for both Hessian and MC PDFs).
The last commit will be a simplification of the N3PDF interface to validphys. And that should be all.
After all this series of PR nothing should be accessing
libNNPDFresults any longer other than the commondata datapoints which are immediately wrapped into aStatsclass:nnpdf/validphys2/src/validphys/results.py
Line 81 in 0321658
but loading them in python is the next step: #1511 :)
(ofc, this last one is not necessary to destroy C++ but since we were already here we might as well do it, if you guys prefer to keep doing MC pdfs as we were doing up to know please let me know asap so I don't build on top of this PR)