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.

Sign up for GitHub

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

Jump to bottom

Utilize stats everywhere where it can be used #1515

Merged
merged 7 commits into from
Mar 1, 2022
Merged

Utilize stats everywhere where it can be used #1515

merged 7 commits into from
Mar 1, 2022

Conversation

scarlehoff
Copy link
Member

I will leave the theory predictions for the end.

@scarlehoff scarlehoff marked this pull request as draft February 9, 2022 18:05
@scarlehoff scarlehoff added destroyingc++ run-fit-bot Starts fit bot from a PR. labels Feb 9, 2022
@scarlehoff scarlehoff added run-fit-bot Starts fit bot from a PR. and removed run-fit-bot Starts fit bot from a PR. labels Feb 10, 2022
Base automatically changed from add_test_w_positivitiy to master February 10, 2022 15:22
@github-actions
Copy link

Greetings from your nice fit 🤖 !
I have good news for you, I just finished my tasks:

Check the report carefully, and please buy me a ☕ , or better, a GPU 😉!

@scarlehoff scarlehoff removed the run-fit-bot Starts fit bot from a PR. label Feb 10, 2022
@scarlehoff
Copy link
Member Author

scarlehoff commented Feb 10, 2022 •
edited
Loading

In the last commit the ThPredictions are no longer using the NNPDFResult class and instead use the StatsResult. Everyting seems to work fine (I'll send the fit bot again now). Since now the central value is computed by taking the average over the result of the members (instead of taking the result of the central replica as the central value) the regression tests have slightly changed. I've uploaded in this comment the diff of the differences for future reference. They are mostly order 1e-3 with some getting to 1e-2.

I will finish what's missing from this PR (mainly making sure that the grid_values are never used as arrays but always as instances of the Stats class) and then will mark this PR for review. This will also make the xplotting grid to be consistent with other grids like Lumi1dGrid for which grid_values is already a Stats instance.

The change of the MC PDF to include by default all replicas (and then being the job of the StatsClass to decide what to do with them) I will do in a separate PR pointing to this one because although is in principle a smaller change (if I did everything correctly it should only be a small change on the behaviour of the Stats and PDF classes) but that could have many unforeseeable consequences.

differences_from_da3069c_to_1fe212432869d.tar.gz

@Zaharid
Copy link
Contributor

Zaharid commented Feb 10, 2022

Good to be rid of the nnpdfresult abomination!

@scarlehoff scarlehoff added the run-fit-bot Starts fit bot from a PR. label Feb 10, 2022
@github-actions
Copy link

Greetings from your nice fit 🤖 !
I have good news for you, I just finished my tasks:

Check the report carefully, and please buy me a ☕ , or better, a GPU 😉!

@scarlehoff scarlehoff mentioned this pull request Feb 11, 2022
Closed
@scarlehoff scarlehoff removed the run-fit-bot Starts fit bot from a PR. label Feb 11, 2022
@scarlehoff scarlehoff added the run-fit-bot Starts fit bot from a PR. label Feb 11, 2022
@Zaharid Zaharid requested a review from RoyStegeman February 11, 2022 13:06
@scarlehoff scarlehoff added run-fit-bot Starts fit bot from a PR. and removed run-fit-bot Starts fit bot from a PR. labels Feb 11, 2022
@github-actions
Copy link

Greetings from your nice fit 🤖 !
I have good news for you, I just finished my tasks:

Check the report carefully, and please buy me a ☕ , or better, a GPU 😉!

@scarlehoff scarlehoff removed the run-fit-bot Starts fit bot from a PR. label Feb 14, 2022
@scarlehoff scarlehoff marked this pull request as ready for review February 14, 2022 11:27
@scarlehoff
Copy link
Member Author

This is ready for review.

At this point the raw rawdata or data is used wherever the _rawdata was used, so the results are expected to be equivalent to whatever we got before this PR. Both the tests and the fitbot seems to confirm that.

In order to continue (and deal with #1516) I will open today another PR pointing to this one where instead of the raw data, the error_members property/method is used wherever it makes sense. The reason for making it another PR is because, as said in #1516, the code is very inconsistent in many places so it can potentially break havoc and I prefer not to increase the entropy of this (large enough) PR.

@scarlehoff scarlehoff mentioned this pull request Feb 15, 2022
Merged
@Zaharid
Copy link
Contributor

Zaharid commented Feb 24, 2022

Apart from the things discussed above I think we should merge this to make some progress.

@scarlehoff
Copy link
Member Author

Apart from the things discussed above I think we should merge this to make some progress.

If you want small things like those (that do not break anything) you can add as a TODO to the third PR, #1522 and I'll do them there.

@scarlehoff scarlehoff merged commit 9613456 into master Mar 1, 2022
@scarlehoff scarlehoff deleted the utilize_stats_rounds_one branch March 1, 2022 11:24
@Zaharid Zaharid added the enhancement New feature or request label May 12, 2022
@Zaharid Zaharid mentioned this pull request May 12, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Zaharid Zaharid Zaharid approved these changes

@RoyStegeman RoyStegeman Awaiting requested review from RoyStegeman

Assignees
No one assigned
Labels
destroyingc++ enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@scarlehoff @Zaharid