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.

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

Les Houches benchmark study 23 #299

Merged
merged 11 commits into from
Oct 10, 2023
Merged

Les Houches benchmark study 23 #299

merged 11 commits into from
Oct 10, 2023

Conversation

felixhekhorn
Copy link
Contributor

Let's do the Les Houches stuff here in this branch (which is based on 0.13)

  • I started drafting the NNLO benchmark, please have a look - how much do we search for precision? i.e. are we fine with our defaults?
  • Commit 36049b3 fixes a bug and contains the necessary fix needed here - if we decide to not merge this PR, this should survive (you may call the change breaking, but I wonder how much in practice)

@felixhekhorn felixhekhorn added the benchmarks Benchmark (or infrastructure) related label Aug 9, 2023
@alecandido
Copy link
Member

  • I started drafting the NNLO benchmark, please have a look - how much do we search for precision? i.e. are we fine with our defaults?

I'd say so: LO is perfect, and the usual accuracy is quite fine. However, we could dump tables as heatmaps (to resemble the same shape of published tables), with a log color-scale, and check if we are more or less fine, attempting a minimal informal quantification of what fine does mean (like most points below 1e-X, where most might be 95% and X=3).

  • Commit 36049b3 fixes a bug and contains the necessary fix needed here - if we decide to not merge this PR, this should survive (you may call the change breaking, but I wonder how much in practice)

Fine by me, I'm also planning to drop usage of apply_pdf in evolven3fit_new #408, and there are not many other users out there.

@giacomomagni
Copy link
Collaborator

Thanks for drafting it. Should I use this branch to include the update on the 2 splitting functions, maybe a different one is neater?

@alecandido
Copy link
Member

It's more or less fine both ways: you can do it here, or, if you prefer to keep things separated, do it in another branch, and then we'll rebase this on the new one :)

@felixhekhorn
Copy link
Contributor Author

Should I use this branch to include the update on the 2 splitting functions, maybe a different one is neater?

For the private stuff we need to use a private fork (e.g. based on this branch)

@giacomomagni
Copy link
Collaborator

Ah good point that stuff is private...

@alecandido
Copy link
Member

Ah, you're right, we even discussed it. But I forgot immediately the privacy issue...

@giacomomagni
Copy link
Collaborator

Can we merge this?

@alecandido
Copy link
Member

alecandido commented Sep 13, 2023

Fine by me: the exercise code is useful to have in master, since it has already been used to produce results, and everything else is just a small net improvement (fewer qed params around make me happier).

Unless @felixhekhorn is planning to modify the code of the scripts anytime soon, I'd be in favor of merging.

@felixhekhorn
Copy link
Contributor Author

fine merging it ... but I expect we will at some point need an extension, e.g. PTO is hardcoded here ... how about your scripts @giacomomagni ? (like the "actual" N3LO comparison plot)

@giacomomagni
Copy link
Collaborator

they are all in the private eko, because of FHMV https://github.com/NNPDF/eko_fhmv_private/pull/1

@giacomomagni
Copy link
Collaborator

giacomomagni commented Sep 20, 2023

I think this is the best place to introduce PTO_match which is going to differentiate the order of the matching conditions.

@alecandido
Copy link
Member

Fine to introduce the matching order on top of this PR, but could you just split in a separate one? (just pointing to this)

@giacomomagni
Copy link
Collaborator

giacomomagni commented Sep 20, 2023

Fine to introduce the matching order on top of this PR, but could you just split in a separate one? (just pointing to this)

I think this feature is actually needed also for the LH benchmarks as in the last meeting we agreed in VFNS N3LO with NNLO matching. Moreover all the branches of eko_private are based on this and the required changes of this new feature are not that complex.

@felixhekhorn
Copy link
Contributor Author

I think this is the best place to introduce PTO_match which is going to differentiate the order of the matching conditions.

just to tag the corresponding issue: this is the first part of #146

@alecandido
Copy link
Member

I think this feature is actually needed also for the LH benchmarks as in the last meeting we agreed in VFNS N3LO with NNLO matching. Moreover all the branches of eko_private are based on this and the required changes of this new feature are not that complex.

My point was not that we do not need this feature here, but rather the opposite: we do not need the benchmark for the feature.
So, the moment the feature is completed, we can even merge it before the benchmark (rebasing on master, and rebasing this on master after the merge).

I'm just trying to keep PRs as small as possible, to simplify the review process and merge faster (such that we also have more atomic changes in master).

Copy link
Collaborator

@giacomomagni giacomomagni left a comment

Choose a reason for hiding this comment

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

Can we merge this ?

Copy link
Member

@alecandido alecandido left a comment

Choose a reason for hiding this comment

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

Just one comment, but the PR is practically ready to be merged.

src/eko/io/runcards.py Outdated Show resolved Hide resolved
src/eko/runner/parts.py Outdated Show resolved Hide resolved
src/eko/runner/legacy.py Outdated Show resolved Hide resolved
giacomomagni and others added 2 commits October 9, 2023 16:04
Co-authored-by: Alessandro Candido <[email protected]>
Co-authored-by: Alessandro Candido <[email protected]>
@giacomomagni giacomomagni marked this pull request as ready for review October 9, 2023 14:46
@alecandido
Copy link
Member

Actually @felixhekhorn is the author of the PR, so he can not approve. And I already approved (@giacomomagni as well).

Shall we just merge this?

@giacomomagni giacomomagni merged commit 49b8b12 into master Oct 10, 2023
6 checks passed
@giacomomagni giacomomagni deleted the lh-bench-23 branch October 10, 2023 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
benchmarks Benchmark (or infrastructure) related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants