Skip to content

reactivate use_t0_sampling #2300

Merged
scarlehoff merged 25 commits intomasterfrom
reactivate_use_t0_sampling
May 13, 2025
Merged

reactivate use_t0_sampling #2300
scarlehoff merged 25 commits intomasterfrom
reactivate_use_t0_sampling

Conversation

@comane
Copy link
Member

@comane comane commented Mar 17, 2025

This PR reintroduces the option of generating MC pseudo-data using the t0 covariance matrix

Note: this feature was removed here: #1626

Report:
Compares an nnpdf fit done with t0-covmat sampling against a baseline one

https://vp.nnpdf.science/DMbQZIFqTwecA_N6L5XycQ==/

Iterated fits
https://vp.nnpdf.science/DJ4VtvTZQ5O8fZsl3pSDGQ==

  • Fix tests failing because of new use_t0_sampling default

@comane comane changed the title use_t0_sampling in explicit node of production rule reactivate use_t0_sampling Mar 17, 2025
@RoyStegeman
Copy link
Member

RoyStegeman commented Mar 17, 2025

Please document it and include it in one of the example runcards

@comane
Copy link
Member Author

comane commented Mar 19, 2025

Please document it and include it in one of the example runcards

Hi @RoyStegeman, where would you add this in the documentation?

@RoyStegeman
Copy link
Member

RoyStegeman commented Mar 19, 2025

Have a look at the docs and see where you think it should go or fits best. Probably in the n3fit runcard section (which is already incomplete) is enough, but maybe I missed something

@scarlehoff
Copy link
Member

So, as far as I can see this will become the default from now on.

@RoyStegeman
Copy link
Member

So, as far as I can see this will become the default from now on.

Why? Did I miss a discussion about this? Stefano was always in favor of sampling exp so does he agree with doing this now?

@comane
Copy link
Member Author

comane commented Mar 19, 2025

So, as far as I can see this will become the default from now on.

Why? Did I miss a discussion about this? Stefano was always in favor of sampling exp so does he agree with doing this now?

During the code meeting today, SF was in favour of having t0 as sampling covmat.

@scarlehoff
Copy link
Member

scarlehoff commented Mar 19, 2025

Stefano was always in favor of sampling exp so does he agree with doing this now?

fwiw, he was not completely convinced about the toy model but since even in the extreme toy model case the difference was sub per-mille he said it was academic at that point (while making things easier for the diagonal covmat)

@RoyStegeman
Copy link
Member

Okay good. It also makes things easier for TCM so I'm happy with this

@RoyStegeman
Copy link
Member

When do you plan to fix and merge this?

@scarlehoff
Copy link
Member

scarlehoff commented Mar 24, 2025

Remember that for the fit regression tests you can use the label redo-regression

(also, please run the fitbot and update the reference)

@comane comane added the redo-regressions Recompute the regression data label Mar 24, 2025
@RoyStegeman RoyStegeman force-pushed the reactivate_use_t0_sampling branch from d8b7848 to c3a29a9 Compare March 24, 2025 13:12
@RoyStegeman RoyStegeman added the run-fit-bot Starts fit bot from a PR. label Mar 24, 2025
@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
Copy link
Member

A byproduct of using t0 for the sampling is that now generating pseudodata requires a predictions with some theory, which in turns require cuts.

Some of the tests should be modified to set use_t0_sampling: False (and some might need to be updated manually, some of the regression tests in validphys/n3fit might fall into this category).

@scarlehoff
Copy link
Member

I'm also worried that the fitbot here produced exactly the same result as in this other branch https://vp.nnpdf.science/WUyHqXh5QsSxY24iIxQHxw== ?

@RoyStegeman
Copy link
Member

What is the status on this? Since it was agreed that this is what should be done going forwards there is no need to keep it out of master for so long

Copy link
Member

@scarlehoff scarlehoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Just added two requests for tests.

@scarlehoff scarlehoff added run-fit-bot Starts fit bot from a PR. and removed run-fit-bot Starts fit bot from a PR. redo-regressions Recompute the regression data labels May 12, 2025
@scarlehoff
Copy link
Member

When the new fitbot finishes, please update the fitbot reference and I think we can merge?

@comane
Copy link
Member Author

comane commented May 12, 2025

When the new fitbot finishes, please update the fitbot reference and I think we can merge?

Yes, I think that we can merge. What do you mean by updating the fitbot reference?

@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
Copy link
Member

Thanks, after this + the diagonal covmat I think we can start with the 4.1.X tags.

@scarlehoff scarlehoff merged commit 164d33c into master May 13, 2025
11 checks passed
@scarlehoff scarlehoff deleted the reactivate_use_t0_sampling branch May 13, 2025 09:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

run-fit-bot Starts fit bot from a PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants