Skip to content

Init Ekobox#79

Merged
andreab1997 merged 103 commits into
developfrom
feature/init-ekobox
Mar 8, 2022
Merged

Init Ekobox#79
andreab1997 merged 103 commits into
developfrom
feature/init-ekobox

Conversation

@felixhekhorn
Copy link
Copy Markdown
Collaborator

@felixhekhorn felixhekhorn commented Nov 16, 2021

  • strip apply_pdf from Output
  • dump LHAPDF files from eko
  • import eko_identity from pineko
  • import ekos product from @giacomomagni scripts

@felixhekhorn felixhekhorn added enhancement New feature or request refactor Refactor code labels Nov 16, 2021
@alecandido
Copy link
Copy Markdown
Collaborator

Thank you, this we'll make us closer to usability, and then it'll be good for production (even if maybe not strictly required for publication).

Comment thread benchmarks/ekomark/plots.py Outdated
Comment thread benchmarks/ekomark/plots.py Outdated
Comment thread src/ekobox/templatePDF.info Outdated
Comment thread src/ekobox/gen_op.py Outdated
Comment thread src/ekobox/gen_op.py Outdated
Comment thread src/ekobox/gen_theory.py Outdated
Comment thread src/ekobox/gen_theory.py Outdated
Comment thread tests/test_ekobox_gen_theory.py Outdated
@andreab1997
Copy link
Copy Markdown
Collaborator

I was wondering if also ekobox needs a CLI. What do you think? @alecandido @felixhekhorn @giacomomagni

@alecandido
Copy link
Copy Markdown
Collaborator

I was wondering if also ekobox needs a CLI. What do you think? @alecandido @felixhekhorn @giacomomagni

We already thought about an eko CLI, and maybe this will actually be called eko and implemented inside ekobox, but consider the next three items:

  1. The purpose of ekobox is to support mainly notebook programming: for users plots are more meaningful, and whatever data is better to see in pandas dataframes, so notebook are the perfect match (we are usually using navigator and scroll through numbers, but this is mostly interesting for an eko developer I guess)
  2. The CLI is fine, but should support only 2 operations:
    • given theory and operator cards, generate the corresponding operator (in the main output format, i.e. tar archive)
    • given theory and operator cards, and an initial PDF, generate and dump the corresponding LHAPDF grid
  3. No NNPDF project will directly use CLI, at least for the time being, since FkTable generation will use evolution through pineko (thus making use of python API), and most likely even fit evolution will be managed by vp (that is another python code and will make use of python API) -> so it's not urgent

@andreab1997
Copy link
Copy Markdown
Collaborator

I was wondering if also ekobox needs a CLI. What do you think? @alecandido @felixhekhorn @giacomomagni

We already thought about an eko CLI, and maybe this will actually be called eko and implemented inside ekobox, but consider the next three items:

  1. The purpose of ekobox is to support mainly notebook programming: for users plots are more meaningful, and whatever data is better to see in pandas dataframes, so notebook are the perfect match (we are usually using navigator and scroll through numbers, but this is mostly interesting for an eko developer I guess)

  2. The CLI is fine, but should support only 2 operations:

    • given theory and operator cards, generate the corresponding operator (in the main output format, i.e. tar archive)
    • given theory and operator cards, and an initial PDF, generate and dump the corresponding LHAPDF grid
  3. No NNPDF project will directly use CLI, at least for the time being, since FkTable generation will use evolution through pineko (thus making use of python API), and most likely even fit evolution will be managed by vp (that is another python code and will make use of python API) -> so it's not urgent

OK so eventually the CLI will be for ekobox and eko together -> it should wait

@alecandido
Copy link
Copy Markdown
Collaborator

OK so eventually the CLI will be for ekobox and eko together -> it should wait

If you feel like you can even start, simply at the moment is not a priority (but for as long as you are able to, please keep choosing things you prefer to do :) ).

@alecandido
Copy link
Copy Markdown
Collaborator

@andreab1997 : did you commit the cached_out by chance or intentionally?

@andreab1997
Copy link
Copy Markdown
Collaborator

@andreab1997 : did you commit the cached_out by chance or intentionally?

Intentionally. I provided the possibility to evolve using a cached output and I was testing it (this helps also to make the test of CT14 faster).

@alecandido
Copy link
Copy Markdown
Collaborator

Keep an eye at pylint, you can see the report diff directly on codefactor

@felixhekhorn felixhekhorn linked an issue Jan 25, 2022 that may be closed by this pull request
@alecandido
Copy link
Copy Markdown
Collaborator

I imported eko_identity, such that I can remove it from pineko (where it is not used any longer).

But this PR is getting old, and so annoying to update with develop. Let's get this last point within next week and merge what we have (or even this week if @andreab1997 is available, but no pressure and priority to MHOU).

@alecandido
Copy link
Copy Markdown
Collaborator

I gave up with fancy privileges restrictions inside the container, since it looks like GitHub workflows, when the job is fully inside a container, require to run it as root:
https://docs.github.com/en/actions/creating-actions/dockerfile-support-for-github-actions#user

Since 19ee4c5 the new Isolated benchmarks are actually fully working, and they reproduce local results (the trick has been, unsurprisingly, poetry config virtualenvs.create false).

Now, the situation is the following:

  • the error of first kind (i., see previous comment) is gone, as expected it was only related to a missing full installation of LHAPDF (since it was used through the package), and thus (most likely) missing lhapdf.conf (to solve locally, install LHAPDF by hand in poetry environment poetry env info -p, e.g. from https://github.com/N3PDF/external)
  • the error of the second kind (ii.) is still there, to be solved
  • the error on msbar_masses is back, since now even APFEL is available; maybe we just need to upgrade APFEL version, what do you think @giacomomagni and @felixhekhorn?
  • of course, I forgot to mention an error already known to @andreab1997, i.e. FileNotFoundError: [Errno 2] No such file or directory: '/__w/eko/eko/benchmarks/ekobox/cached_out/o7f0768_te3dbf6.tar', so, most likely, it needs to be regenerated (or we can drop the idea of caching it, and just generate online, since it's no longer a unit test)

Solved this 3 errors, plus completing the unit tests coverage on eko (and only eko), it's enough to get back the green tick on workflows and codecov, and enough for me to merge.
Final caveats:

  • for the coverage see above, it might be enough to delete some old code
  • for the full green tick we would also need to satisfy Pylint (this is the last thing I'll try to take care myself); I don't consider this needed to merge

P.S.: I start to believe this branch to be cursed, let's close it as soon as possible

@andreab1997
Copy link
Copy Markdown
Collaborator

  • of course, I forgot to mention an error already known to @andreab1997, i.e. FileNotFoundError: [Errno 2] No such file or directory: '/__w/eko/eko/benchmarks/ekobox/cached_out/o7f0768_te3dbf6.tar', so, most likely, it needs to be regenerated (or we can drop the idea of caching it, and just generate online, since it's no longer a unit test)

Actually, in 039806a I should have fixed that test (I tried and for me it is working). Is failing for you? @alecandido

@alecandido
Copy link
Copy Markdown
Collaborator

Now it's not a matter of local environments: it's failing in the CI, you can have a look at the logs by yourself.

In any case, I would give up on pre-conputed output: everything that is stored has to be kept up to date, and we know that Eko is going to change (including the output format itself).

Online computation will ensure consistency.

@andreab1997
Copy link
Copy Markdown
Collaborator

Now it's not a matter of local environments: it's failing in the CI, you can have a look at the logs by yourself.

In any case, I would give up on pre-conputed output: everything that is stored has to be kept up to date, and we know that Eko is going to change (including the output format itself).

Online computation will ensure consistency.

ok, perfect

@giacomomagni
Copy link
Copy Markdown
Collaborator

giacomomagni commented Mar 7, 2022

I guess it's still due to the fact that apfel is not available because of the environment (so failing when apfel available); here @giacomomagni might help, my hypothesis is that these numbers have been obtained with a patched version of apfel, and not v3.0.5

Yes indeed, it looks the failure is due to top evolution (Q=1) which was the bugged one. This bug was fixed by Valerio, so upgrading Apfel should solve the issue no?

@andreab1997 andreab1997 merged commit 3b843e3 into develop Mar 8, 2022
@delete-merged-branch delete-merged-branch Bot deleted the feature/init-ekobox branch March 8, 2022 08:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request refactor Refactor code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Recover plot operators

4 participants