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 IO for LHA bot #238

Merged
merged 3 commits into from
Apr 4, 2023
Merged

Fix IO for LHA bot #238

merged 3 commits into from
Apr 4, 2023

Conversation

felixhekhorn
Copy link
Contributor

@felixhekhorn felixhekhorn commented Apr 3, 2023

Closes #237

This PR fixes

  • Runcard update broken #237 and adds the relevant test
  • a inconsistent translation of mu2grid -> mugrid in LHA
  • the output of flavored_mugrid to be serializable and adds the relevant test
  • left-over Q2grid in LHA benchmark
  • left-over Q2grid in navigator
  • left-over Q2grid in docs

further comments:

  • Runcard update broken #237 also affects yadism of course
  • the current implementation looses track of QrefQED since there is no such thing any longer - what should we do? should we translate the existence to couplings.em_running? should we change the theory again? (meaning banana)
  • by creating this PR we can test the trigger for Simple LHA benchmark workflow #227

@felixhekhorn felixhekhorn added bug Something isn't working benchmarks Benchmark (or infrastructure) related labels Apr 3, 2023
src/eko/io/runcards.py Outdated Show resolved Hide resolved
@felixhekhorn
Copy link
Contributor Author

@alecandido the trigger for #227 is still not optimal, I think ... run 14 and 15 have been triggered simultaneously and I suspect by me asking for 2 reviewers - but of course we need only one run, regardless of how many people I ask

@alecandido
Copy link
Member

@alecandido the trigger for #227 is still not optimal, I think ... run 14 and 15 have been triggered simultaneously and I suspect by me asking for 2 reviewers - but of course we need only one run, regardless of how many people I ask

@felixhekhorn true, but I believe it should do so if you select them together. You can select multiple reviewer in a single shot, without exiting the dropdown menu, but if you exit, you're filing two requests in a row (even if they are later on grouped in the PR board).
I'm not sure, it is just a hypothesis, and I know it is a bit tricky to select multiple reviewers without closing the menu. But this is a UI issue, not a trigger problem (I believe).

@alecandido
Copy link
Member

The fail_ci_if_error argument is actually working!
https://github.com/NNPDF/eko/actions/runs/4605985451/jobs/8138726474?pr=238#step:12:40

@felixhekhorn
Copy link
Contributor Author

The fail_ci_if_error argument is actually working! https://github.com/NNPDF/eko/actions/runs/4605985451/jobs/8138726474?pr=238#step:12:40

yes! 🎉

PS: 882487b is the same of f9ee63f

@alecandido
Copy link
Member

PS: 882487b is the same of f9ee63f

Right, I will rebase immediately!

@alecandido alecandido force-pushed the fix-runcards-lhabot branch from 882487b to 51d66ac Compare April 4, 2023 10:07
@alecandido
Copy link
Member

I didn't even have to drop it manually: they were the same diff, and Git figured out on its own he didn't have to reapply it twice.

@alecandido
Copy link
Member

@felixhekhorn as soon as the workflow will pass again, I believe we can merge!

@felixhekhorn
Copy link
Contributor Author

  • How about the QED issue?
  • we still have the 100% error in github:
Computing for theory=45286e9, ocard=cc12964 and pdf=ToyLH ...
Compute external result

Log added, hash=2e5273d953ef355a2f48badae3729333e94e6808ae941048e2e2e943771b0e63


 ─── 
  S  
 ─── 
               x       Q2         eko     eko_error         LHA  percent_error
0   1.000000e-07  10000.0    0.000000  0.000000e+00  251.422304    -100.000000
1   1.000000e-06  10000.0  132.769958  1.641839e-04  133.133286      -0.272906

but I cannot reproduce this locally

In [2]: dfl(-1)
Out[2]: 

- theory: `45286e92f0fbedc3ed4a84b47547190efc4bde84583e5782716a5c5c70448aed`
- obs: `8beae75234d8ba49de7079e5f76d3916dd810708d31bb54f85505faf1c470055`
- using PDF: *ToyLH*

S
               x       Q2         eko     eko_error         LHA  percent_error
0   1.000000e-07  10000.0  250.741696  3.320808e-04  251.422304      -0.270703
1   1.000000e-06  10000.0  132.769958  1.641844e-04  133.133286      -0.272906

Note that the obs hash is not the same ... I seem to remember to have seen this before ... @giacomomagni ? or @t7phy ?

@alecandido
Copy link
Member

@felixhekhorn I acknowledge that to be an issue, but it is not #237

With the idea of keeping PRs short and atomic, this has already solved its related issue, and it already provides a useful result, even though not perfect.
Let's merge it, and open a separate issue to track the problem you are mentioning.

@alecandido
Copy link
Member

#239 has been opened for the remaining issue, please merge this (and make sure it will close #237).

@felixhekhorn
Copy link
Contributor Author

#239 has been opened for the remaining issue, please merge this (and make sure it will close #237).

Even fine, but then let's not mix the two distinct problems #239 and #241

@felixhekhorn felixhekhorn merged commit 8f0e3a7 into master Apr 4, 2023
@felixhekhorn felixhekhorn deleted the fix-runcards-lhabot branch April 4, 2023 10:58
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Runcard update broken
2 participants