Provide some tests

[andreab1997]

We want to provide some unit test for pineko

[codecov]

Codecov Report

Merging #25 (1774ae5) into main (9923b5c) will increase coverage by 40.01%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##             main      #25       +/-   ##
===========================================
+ Coverage   44.03%   84.05%   +40.01%     
===========================================
  Files          16       16               
  Lines         604      602        -2     
===========================================
+ Hits          266      506      +240     
+ Misses        338       96      -242     

Flag	Coverage Δ
bench	82.50% <100.00%> (?)
unittests	47.00% <0.00%> (+2.97%)	⬆️

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

Impacted Files	Coverage Δ
src/pineko/cli/_base.py	100.00% <ø> (ø)
src/pineko/evolve.py	89.09% <100.00%> (+63.63%)	⬆️
src/pineko/check.py	93.10% <0.00%> (+13.79%)	⬆️
src/pineko/cli/opcard.py	100.00% <0.00%> (+15.38%)	⬆️
src/pineko/cli/compare.py	100.00% <0.00%> (+16.66%)	⬆️
src/pineko/cli/check.py	89.13% <0.00%> (+45.65%)	⬆️
src/pineko/theory.py	69.94% <0.00%> (+50.28%)	⬆️
src/pineko/parser.py	91.30% <0.00%> (+60.86%)	⬆️
src/pineko/theory_card.py	84.61% <0.00%> (+65.38%)	⬆️
... and 2 more

[felixhekhorn]

• Very good your start this project ;-) however, if you feel it is easier to do benchmarks before unit tests (something I would do) feel free to do so ...
• @alecandido I guess we want to keep the split between unit tests in tests that have no external dependencies and benchmarks?
• benchmarks will be required (at some point) as they test the actual thing - specific here: the interplay with eko, lhapdf as pineko really is a glue ...

[andreab1997]

• Very good your start this project ;-) however, if you feel it is easier to do benchmarks before unit tests (something I would do) feel free to do so ...
• @alecandido I guess we want to keep the split between unit tests in tests that have no external dependencies and benchmarks?
• benchmarks will be required (at some point) as they test the actual thing - specific here: the interplay with eko, lhapdf as pineko really is a glue ...

Yes, I agree that benchmarks is what we really need. However I would start doing unit tests to understand better how the code works before starting to write benchmarks.

[alecandido]

Yes, I agree that benchmarks is what we really need. However I would start doing unit tests to understand better how the code works before starting to write benchmarks.

Benchmark will be useful at some point, and I want to keep the split.

Nevertheless, what we really need are unit tests: they are useful not only as a consistency check (like benchmarks), but also to lead development and refactoring (à la TDD, we have already seen them in action).

As usual, LHAPDF will make a bit more cumbersome to install dependencies, but maybe I can try to properly repackage it, and update on PyPI (and we might avoid a further container this way).

[andreab1997]

@alecandido @felixhekhorn I have some doubts about the function https://github.com/N3PDF/pineko/blob/20e6c5765afc43eafd05468101d54dc997631755/src/pineko/check.py#L6
I tried to construct a test basically equal to the one I pushed but with an additional entry in the to_check array and the test failed. So I looked into the function and, for example, I did not understand why there is a[1,-1] in the place of a.
Moreover in the numpy documentation it is said that the first array passed to searchsorted must be ordered but this is not enforced anywhere. I also tried my test with the native numpy function np.in1d and it worked.
Now, I did not touch the function because it seems that for the actual use cases it is working (in fact the second test is working properly) but I wonder if you understand better what is going on.

[alecandido]

So I looked into the function and, for example, I did not understand why there is a[1,-1] in the place of a.

As cited in the docstring, the function is taken from here:
numpy/numpy#7784 (comment)

It is just a trick to retrieve the indices to compare to the elements, and e.g. in case b[0] <= a[1] you want index 0, to later compare to a[0] and a[1].

I'm pretty confident it is going to work for a sorted array, but it might even work for an unsorted one...

The only way to understand it is analyzing a few cases, i.e. testing it:

• tests don't have to be realistic, they are "unit tests", so they are testing the behavior of the function in isolation
• even if you are getting coverage, it might well be that the only way to be confident enough that a function is working, it's testing it with multiple inputs (so, despite coverage, one might be not enough)

[andreab1997]

So I looked into the function and, for example, I did not understand why there is a[1,-1] in the place of a.

As cited in the docstring, the function is taken from here: numpy/numpy#7784 (comment)

It is just a trick to retrieve the indices to compare to the elements, and e.g. in case b[0] <= a[1] you want index 0, to later compare to a[0] and a[1].

I'm pretty confident it is going to work for a sorted array, but it might even work for an unsorted one...

The only way to understand it is analyzing a few cases, i.e. testing it:

• tests don't have to be realistic, they are "unit tests", so they are testing the behavior of the function in isolation
• even if you are getting coverage, it might well be that the only way to be confident enough that a function is working, it's testing it with multiple inputs (so, despite coverage, one might be not enough)

Yes, that is the point. I have tested it with a not realistic input and it failed. Now I am going to understand why, however, even if it is working with realistic inputs, maybe we don't want to keep a function that fails in some cases.

[alecandido]

Yes, that is the point. I have tested it with a not realistic input and it failed. Now I am going to understand why, however, even if it is working with realistic inputs, maybe we don't want to keep a function that fails in some cases.

Definitely we don't. So the correct behavior is keeping the tests, update the function :)
(e.g. you can sort the array, and store the inverse sorting to compare)

[alecandido]

In general, unit tests should test things in isolation. So, please, do your best in order to limit the amount of non-source files you are using in test.

In particular: do your best to remove completely the test_files folder. It is awful to have all these files committed in the repo, and the more files you have, the more things you have to keep updated: if the programs generating them will change, it might change the specific format and content of the files.

The best pattern would be to make fixtures, that are generating required files at runtime, in temporary folders (if really needed).

[andreab1997]

In general, unit tests should test things in isolation. So, please, do your best in order to limit the amount of non-source files you are using in test.

In particular: do your best to remove completely the test_files folder. It is awful to have all these files committed in the repo, and the more files you have, the more things you have to keep updated: if the programs generating them will change, it might change the specific format and content of the files.

The best pattern would be to make fixtures, that are generating required files at runtime, in temporary folders (if really needed).

Yes, I would have liked to do everything at runtime. However I need at least one eko to test and it takes forever to compute it at runtime, even for small datasets

[felixhekhorn]

since we're going for pytest, let's use it! i.e.

• move utils in conftest and make lhapdf_path a fixture (as we do in eko)
• make test_files + test_pdf a fixture
• I also suggest a fixture def grid_path(id) and similar
• a minor style comment: let's move file names from suffix to prefix as we do for tests and as in eko

then let's merge!

[alecandido]

• I also suggest a fixture def grid_path(id) and similar

This I'm not sure is really useful: they are a bunch of functions only used a few times each. However, I'm not strongly opinionated, do whatever you prefer, but don't spend too much time, it is already good enough.

[andreab1997]

• I also suggest a fixture def grid_path(id) and similar

This I'm not sure is really useful: they are a bunch of functions only used a few times each. However, I'm not strongly opinionated, do whatever you prefer, but don't spend too much time, it is already good enough.

Actually I was thinking about the same thing. I believe that the lhapdf_path fixture is useful and the test_pdf and test_files ones are useful as well but a grid_path fixture I don't think it would be. So for me we can merge, do you agree?

[alecandido]

So for me we can merge, do you agree?

Almost, find and replace: s/theory_obj_Hera/theory_obj_hera/g in the whole bench_theory.py. You did only for the definition, and it is obviously breaking the rest...

[andreab1997]

So for me we can merge, do you agree?

Almost, find and replace: s/theory_obj_Hera/theory_obj_hera/g in the whole bench_theory.py. You did only for the definition, and it is obviously breaking the rest...

Yes, I know :)

[andreab1997]

Actually now docker is failing for some reason...

[alecandido]

Actually now docker is failing for some reason...

Network/server issue, I believe. Let's retry with the new commit, otherwise again in a while.

[alecandido]

N3PDF/external is currently private: either we make it public, or we should move the containers somewhere else.

Given that there is little to nothing in external (just the install scripts, and the Containerfiles) at this point I propose to move the whole external in N3PDF/workflows (since those scripts and so on are in the first place useful for CI).
Moreover, we do not make the same use of external as for NNPDF/external, since we are not hosting the code any longer.

Do you agree? @felixhekhorn @andreab1997 @giacomomagni

[codecov-commenter]

Codecov Report

Merging #25 (584e653) into main (9923b5c) will increase coverage by 40.01%.
The diff coverage is 100.00%.

Additional details and impacted files

@@             Coverage Diff             @@
##             main      #25       +/-   ##
===========================================
+ Coverage   44.03%   84.05%   +40.01%     
===========================================
  Files          16       16               
  Lines         604      602        -2     
===========================================
+ Hits          266      506      +240     
+ Misses        338       96      -242     

Flag	Coverage Δ
bench	82.50% <100.00%> (?)
unittests	47.00% <0.00%> (+2.97%)	⬆️

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

Impacted Files	Coverage Δ
src/pineko/cli/_base.py	100.00% <ø> (ø)
src/pineko/evolve.py	89.09% <100.00%> (+63.63%)	⬆️
src/pineko/check.py	93.10% <0.00%> (+13.79%)	⬆️
src/pineko/cli/opcard.py	100.00% <0.00%> (+15.38%)	⬆️
src/pineko/cli/compare.py	100.00% <0.00%> (+16.66%)	⬆️
src/pineko/cli/check.py	89.13% <0.00%> (+45.65%)	⬆️
src/pineko/theory.py	69.94% <0.00%> (+50.28%)	⬆️
src/pineko/parser.py	91.30% <0.00%> (+60.86%)	⬆️
src/pineko/theory_card.py	84.61% <0.00%> (+65.38%)	⬆️
... and 2 more

[andreab1997]

Now that the workflow is running correctly, can we merge this? @alecandido @felixhekhorn