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

ekobox unittests #109

Merged
merged 20 commits into from
Mar 25, 2022
Merged

Conversation

felixhekhorn
Copy link
Contributor

  • moved some code from benchmarks back to tests (those unrelated to lhapdf)

@felixhekhorn felixhekhorn added enhancement New feature or request refactor Refactor code labels Mar 21, 2022
@codecov
Copy link

codecov bot commented Mar 21, 2022

Codecov Report

Merging #109 (39338e8) into feature/composite-output (64f321b) will increase coverage by 0.09%.
The diff coverage is 100.00%.

Impacted file tree graph

@@                     Coverage Diff                      @@
##           feature/composite-output     #109      +/-   ##
============================================================
+ Coverage                     99.51%   99.60%   +0.09%     
============================================================
  Files                            39       51      +12     
  Lines                          2684     3055     +371     
============================================================
+ Hits                           2671     3043     +372     
+ Misses                           13       12       -1     
Flag Coverage Δ
unittests 99.60% <100.00%> (+0.09%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/eko/member.py 100.00% <ø> (ø)
src/ekobox/genpdf/flavors.py 100.00% <ø> (ø)
src/ekobox/__init__.py 100.00% <100.00%> (ø)
src/ekobox/evol_pdf.py 100.00% <100.00%> (ø)
src/ekobox/genpdf/__init__.py 100.00% <100.00%> (ø)
src/ekobox/genpdf/export.py 100.00% <100.00%> (ø)
src/ekobox/genpdf/load.py 100.00% <100.00%> (ø)
src/ekobox/info_file.py 100.00% <100.00%> (ø)
src/ekobox/operators_card.py 100.00% <100.00%> (ø)
src/ekobox/theory_card.py 100.00% <100.00%> (ø)
... and 13 more

@felixhekhorn felixhekhorn marked this pull request as ready for review March 24, 2022 10:10
@felixhekhorn felixhekhorn requested review from alecandido and andreab1997 and removed request for alecandido March 24, 2022 10:43
@felixhekhorn
Copy link
Contributor Author

@andreab1997 please take a look to ekobox and tell me what you think ... I did some renaming (e.g. gen_theory.gen_theory_card -> theory_card.generate) so you would most likely need to adjust the n3fit fit side ...

also I'm not really convinced of providing default runcards from banana: 1. are we really sure we want to do this? (instead of sticking to our default-less scheme - and in NNPDF we always have full runcards) and 2. if still, I don't think we should inherit from banana: our default theory there is FFNS3 which is good for debugging, but stupid in production ...

@andreab1997
Copy link
Contributor

@andreab1997 please take a look to ekobox and tell me what you think ... I did some renaming (e.g. gen_theory.gen_theory_card -> theory_card.generate) so you would most likely need to adjust the n3fit fit side ...

Yes I'll take a look. Probably is only a matter of changing some names in evolven3fit.

also I'm not really convinced of providing default runcards from banana: 1. are we really sure we want to do this? (instead of sticking to our default-less scheme - and in NNPDF we always have full runcards) and 2. if still, I don't think we should inherit from banana: our default theory there is FFNS3 which is good for debugging, but stupid in production ...

I have not a strong opinion on that. I believe that for users having some defaults is good because most of them would only use a few of the runcards options. However maybe we can provide less and more "standard" defaults.

@andreab1997
Copy link
Contributor

@andreab1997 please take a look to ekobox and tell me what you think ... I did some renaming (e.g. gen_theory.gen_theory_card -> theory_card.generate) so you would most likely need to adjust the n3fit fit side ...

also I'm not really convinced of providing default runcards from banana: 1. are we really sure we want to do this? (instead of sticking to our default-less scheme - and in NNPDF we always have full runcards) and 2. if still, I don't think we should inherit from banana: our default theory there is FFNS3 which is good for debugging, but stupid in production ...

Anyway, I really like the new names. The old ones were very confusing

@felixhekhorn
Copy link
Contributor Author

Anyway, I really like the new names. The old ones were very confusing

you know whom to blame 🤣 😇

@andreab1997
Copy link
Contributor

Anyway, I really like the new names. The old ones were very confusing

you know whom to blame 🤣 😇

yes, myself! xD

@andreab1997
Copy link
Contributor

@andreab1997 please take a look to ekobox and tell me what you think ... I did some renaming (e.g. gen_theory.gen_theory_card -> theory_card.generate) so you would most likely need to adjust the n3fit fit side ...

also I'm not really convinced of providing default runcards from banana: 1. are we really sure we want to do this? (instead of sticking to our default-less scheme - and in NNPDF we always have full runcards) and 2. if still, I don't think we should inherit from banana: our default theory there is FFNS3 which is good for debugging, but stupid in production ...

I changed the names on evolven3fit and everything should work again

@felixhekhorn
Copy link
Contributor Author

I changed the names on evolven3fit and everything should work again

Mmm that might have been premature ... you maybe want to stash that commit until this branch is released on main, which might take a while ...

@andreab1997
Copy link
Contributor

I changed the names on evolven3fit and everything should work again

Mmm that might have been premature ... you maybe want to stash that commit until this branch is released on main, which might take a while ...

Oh I see. Yes I will do that

@felixhekhorn
Copy link
Contributor Author

@alecandido maybe consider to merge this PR first before continuing on #105

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.

I reviewed half, and never run. I'll do both, but for the time being I submit current comments.

benchmarks/ekobox/benchmark_evol_pdf.py Show resolved Hide resolved
src/ekobox/__init__.py Show resolved Hide resolved
src/ekobox/evol_pdf.py Show resolved Hide resolved
src/ekobox/info_file.py Outdated Show resolved Hide resolved
src/ekobox/operators_card.py Show resolved Hide resolved
src/ekobox/operators_card.py Show resolved Hide resolved
src/ekobox/operators_card.py Outdated Show resolved Hide resolved
src/ekobox/theory_card.py Outdated Show resolved Hide resolved
@alecandido
Copy link
Member

For me this is fine, I'm going to merge and continue the development on the other branch.

@alecandido alecandido merged commit ad3c1d8 into feature/composite-output Mar 25, 2022
@delete-merged-branch delete-merged-branch bot deleted the feature/ekobox-unittests branch March 25, 2022 19:51
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.

3 participants