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

Fix apfel benchmarks #274

Merged
merged 17 commits into from
Jun 16, 2023
Merged

Fix apfel benchmarks #274

merged 17 commits into from
Jun 16, 2023

Conversation

niclaurenti
Copy link
Contributor

Fixing apfel benchmarks. Tomorrow I will run also the other benchmarks to see if they are broken or not

@felixhekhorn felixhekhorn added bug Something isn't working benchmarks Benchmark (or infrastructure) related output Output format and management labels May 23, 2023
@felixhekhorn
Copy link
Contributor

src/eko/io/runcards.py Outdated Show resolved Hide resolved
@felixhekhorn
Copy link
Contributor

Actually this is in conflict with #284 we need to decide a merge order - and since this is older: what are we missing to merge?

@niclaurenti
Copy link
Contributor Author

niclaurenti commented Jun 15, 2023

Actually this is in conflict with #284 we need to decide a merge order - and since this is older: what are we missing to merge?

Sorry I have seen it now. The problem is in

np.testing.assert_allclose(q2, q2a)

for some reason the q values are in wrong order, i.e. ext["values"] has first Q=31 and then Q=100 while in pdf_grid.items() it was the opposite

edit: your commit fixed it

@felixhekhorn
Copy link
Contributor

edit: your commit fixed it

indeed - I like the fix in #284 better

@niclaurenti
Copy link
Contributor Author

@felixhekhorn @alecandido In the apfel benchmarks (and also in banana) we still have QrefQED that is no longer present in eko. My proposal was to restore Qedref to make it compatible with the nnpdf runcards and use it to switch on and off the em_running

@alecandido
Copy link
Member

@felixhekhorn @alecandido In the apfel benchmarks (and also in banana) we still have QrefQED that is no longer present in eko. My proposal was to restore Qedref to make it compatible with the nnpdf runcards and use it to switch on and off the em_running

Fine by me, for as long as it is not tracking EKO and yadism it is reasonable not to implement a third convention

@felixhekhorn
Copy link
Contributor

@felixhekhorn @alecandido In the apfel benchmarks (and also in banana) we still have QrefQED that is no longer present in eko. My proposal was to restore Qedref to make it compatible with the nnpdf runcards and use it to switch on and off the em_running

didn't we have this behaviour already at some point of the 1M iterations? aem_running = QrefQED == Qref

@niclaurenti
Copy link
Contributor Author

@felixhekhorn @alecandido In the apfel benchmarks (and also in banana) we still have QrefQED that is no longer present in eko. My proposal was to restore Qedref to make it compatible with the nnpdf runcards and use it to switch on and off the em_running

didn't we have this behaviour already at some point of the 1M iterations? aem_running = QrefQED == Qref

Yes in eko 0.12 it is there. I think that since eko 0.13 it has been removed. Now I will push my proposal and tell me what you think (obviously I have to fix also banana)

@niclaurenti
Copy link
Contributor Author

Last commit shows what I had in mind: I changed nan with 0 since em_running will be enabled only if Qref==Qedref so 0 could be every number different from Qref

@niclaurenti
Copy link
Contributor Author

niclaurenti commented Jun 15, 2023

Last commit shows what I had in mind: I changed nan with 0 since em_running will be enabled only if Qref==Qedref so 0 could be every number different from Qref

Moreover in this way both pineko and evolven3fit_new will use running alpha with theory 523 and fixed alpha with theory 522 without adding some lines that explicitly set em_running=True for example like in

https://github.com/NNPDF/nnpdf/blob/6bfc220a766777e31df9ce242cfd4c584b078a9a/n3fit/src/evolven3fit_new/eko_utils.py#L83

Copy link
Contributor

@felixhekhorn felixhekhorn left a comment

Choose a reason for hiding this comment

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

You need to fix (+ release + update) banana before merging, else we're breaking the benchmarks that are supposed to fix in this PR again

@felixhekhorn
Copy link
Contributor

@niclaurenti you need to run poetry lock in order to get the lock file correct, since there is a hashing algorithm in the game ...

@niclaurenti
Copy link
Contributor Author

for me we can merge

@felixhekhorn
Copy link
Contributor

let's wait for https://github.com/NNPDF/eko/actions/runs/5280648705/jobs/9553070195 ...

PS: can you double check you can run APFEL still (i.e. with new banana)?

@niclaurenti
Copy link
Contributor Author

let's wait for https://github.com/NNPDF/eko/actions/runs/5280648705/jobs/9553070195 ...

PS: can you double check you can run APFEL still (i.e. with new banana)?

It works

@felixhekhorn felixhekhorn merged commit e019ad2 into master Jun 16, 2023
@felixhekhorn felixhekhorn deleted the fix-apfelbench branch June 16, 2023 08:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
benchmarks Benchmark (or infrastructure) related bug Something isn't working output Output format and management
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants