Stabilize APIs

[Radonirinaunimi]

Addresses the API parts of #326 and #327

• Python part:
  • Fix the tutorials
  • stabilize the API
  • increase test coverage
• Fortran part (done modulo some potential memory leaks in examples):
  • stabilize the API (add still missing functions)
  • set examples _v1 as default/main and deprecate the old ones
  • run Fortran examples on the CI
  • fix bugs in the generalized convolutions
• C/C++ part:
  • fix a bug in grid_convolve
  • test convolutions with two different QED PDF sets
• C++ OOP:
  • implement generalized convolution function
  • run C++ OOP examples on the CI

[Radonirinaunimi]

@cschwan In case you want to have a look, this is complete.

[felixhekhorn]

Could you please also adjust https://github.com/NNPDF/pineappl/blob/stabilize-apis/pineappl_capi/include/PineAPPL.hpp ?

[cschwan]

Looks quite good to me, however we can still increase the CAPI code coverage:

• fill a Grid with no reweighting to cover line 124
• call pineappl_grid_key_value with initial_state_1 and initial_state_2 to cover lines 978 to 981 - shouldn't this happen already?
• call pineappl_grid_convolve with mu_scales set to nullptr to cover line 1836

In the Python API we should

• trigger the error paths in Grid.set_bwfl and Grid.merge
• test the boc module more extensively.

[cschwan]

@Radonirinaunimi BTW: the Fortran module file is shipped with PineAPPL and isn't installed. This means we can forgo giving any stability guarantees and just change it as we see fit. If a user wants to use an older version of the module they always can, because the module is just a thin wrapper of the CAPI which is stable. So if you think it's too much of an effort to keep the deprecated functions in the Fortran module you can just strip them out.

The same holds in principle for the C++ OOP API that @felixhekhorn mentioned. I'd even be in favour of either completely removing the C++ OOP API or letting someone else maintain it, possibly in a different repository even, because in C++ you can always use the CAPI without any additional burden. The C++ OOP vs. C functions is really just a stylistic choice. But that's a decision you have to make, @Radonirinaunimi; having 4 different APIs (CAPI, Python, Fortran, C++ OOP) may be too much a maintainance burden.

[felixhekhorn]

I'd even be in favour of either completely removing the C++ OOP API or letting someone else maintain it, possibly in a different repository even, because in C++ you can always use the CAPI without any additional burden. The C++ OOP vs. C functions is really just a stylistic choice.

It is just a convenience wrapper - but personally I would never use the CAPI ;-) it's too dangerous with all the pointers and stuff. I'd rather solve that problem once and for all and then hide the details - that's why I wrote the interface in the first place. I did the same for gsl. If you want to drop it from here, fine - I can move that to my account if needs be.

[Radonirinaunimi]

@Radonirinaunimi BTW: the Fortran module file is shipped with PineAPPL and isn't installed. This means we can forgo giving any stability guarantees and just change it as we see fit. If a user wants to use an older version of the module they always can, because the module is just a thin wrapper of the CAPI which is stable. So if you think it's too much of an effort to keep the deprecated functions in the Fortran module you can just strip them out.

I've actually always wondered why we don't install the Fortran API? We can promote it to a folder pineappl_fapi and add versions to it? Are there reasons against this?

The same holds in principle for the C++ OOP API that @felixhekhorn mentioned. I'd even be in favour of either completely removing the C++ OOP API or letting someone else maintain it, possibly in a different repository even, because in C++ you can always use the CAPI without any additional burden. The C++ OOP vs. C functions is really just a stylistic choice. But that's a decision you have to make, @Radonirinaunimi; having 4 different APIs (CAPI, Python, Fortran, C++ OOP) may be too much a maintainance burden.

As for this, I don't actually mind maintaining these APIs. The current situation is a bit peculiar because v1 added lots of changes, but by now I think the main crate should be much more stable and future changes will not be as significant.

So, yes, I can also update this:

Could you please also adjust https://github.com/NNPDF/pineappl/blob/stabilize-apis/pineappl_capi/include/PineAPPL.hpp?

after all of the following is done: #328 (comment).

[Radonirinaunimi]

test the boc module more extensively.

@cschwan, by this do you mean increasing the areas/percentage covered? Because boc is already extensively tested in test_boc.py. The problem is that I am not sure it is possible (from Python side) to hit the enum such as in line 311.

[cschwan]

I've actually always wondered why we don't install the Fortran API? We can promote it to a folder pineappl_fapi and add versions to it? Are there reasons against this?

There are a few reasons:

1. compiled Fortran module files depend not only on the compiler, but also on the compiler version
2. there's no agreed-upon installation directory where these files would go, see for instance the discussion here: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=49138
3. even if you solve the first two problems, there's no (?) tooling to compile and install the modules, and we should even try to write our tool, which is bound to break for different compilers

[cschwan]

test the boc module more extensively.

@cschwan, by this do you mean increasing the areas/percentage covered? Because boc is already extensively tested in test_boc.py. The problem is that I am not sure it is possible (from Python side) to hit the enum such as in line 311.

We should strive to get as close as possible to 100% coverage. Your last two commits did raise the coverage and I believe that last point you should check is whether PyScales::default is unused (I think it is), and if so remove the method.

[Radonirinaunimi]

[..] I've found a bug in the C bindings that compute pineappl_grid_convolve_with_two and pineappl_grid_convolve. In both, the state was always fixed and therefore always the same PDF is used.

We can change state to be an array of void* that must match the length of xfxs for pineappl_grid_convolve. Since we can't change pineappl_grid_convolve_with_two the user has to take care of this themselves; this can be done with code like the following:

struct State {
    PDF pdfs[2];
};

double xfx1(int32_t x, double x, double q2, void* state) {
    auto* pdf = dynamic_cast <State*> (state)->pdfs[0];
    // ..
}

double xfx2(int32_t x, double x, double q2, void* state) {
    auto* pdf = dynamic_cast <State*> (state)->pdfs[1];
    // ..
}

int main() {
    State state;
    // ..
    pineappl_grid_convolve_with_two(..., xfx1, ..., xfx2, ..., &state);
    // ..
}

Was this what you were worried about? It's not too complicated, but I agree that for the new function we better increase the ergonomics.

Yes, the fix was rather easy (its Fortran interface, not so much).

The only minor difference w.r.t. your suggestion was that I only kept one "true" Lambda xfx and instead passed pdf1_state and pdf2_state. If I had set state to be an array for pineappl_grid_convolve_with_two then that function would be the same as pineappl_grid_convolve.

On the Fortran node, @janw20, would you perhaps be available to help? There are only two functions to fix, pineappl_grid_convolve_with_two and pineappl_grid_convolve but they seem a bit tricky.

[cschwan]

@Radonirinaunimi: you can't change pineappl_grid_convolve_with_two (nor pineappl_grid_convolute_with_two), because this function has been part of the CAPI since 0.8.0. Therefore you'd break existing programs. Remember: once we've added a symbol (function, struct, typedef, ...) in the CAPI that has an official release, we can never change or remove that symbol.

[cschwan]

This is a recommendation, but one to make your life easier: since you've removed the v0 parts of the C++ API you can also remove these parts from the Fortran API; if someone needs them, they just can use an older module file from the git.

[cschwan]

I renamed the enum variants in commit f153ec3, please adjust the other examples accordingly if needed. Also please use the ScreamingSnakeCase names from commit bc021f5 whenever using pineappl_grid_optimize_using.

[Radonirinaunimi]

@Radonirinaunimi: you can't change pineappl_grid_convolve_with_two (nor pineappl_grid_convolute_with_two), because this function has been part of the CAPI since 0.8.0. Therefore you'd break existing programs. Remember: once we've added a symbol (function, struct, typedef, ...) in the CAPI that has an official release, we can never change or remove that symbol.

Yes, that's right. However, this was definitely a bug in the sense that the old functions were computed something completely wrong, and so I though it'd be fine to break the symbol.

So considering the above and the following:

This is a recommendation, but one to make your life easier: since you've removed the v0 parts of the C++ API you can also remove these parts from the Fortran API; if someone needs them, they just can use an older module file from the git.

I believed such a price would be fine (?).

For the record, the changes have been undone in b4bb0f8.

[Radonirinaunimi]

I believe this is now "ready". It seems that there are also some memory leaks in the Fortran examples that I am not able to really track (apart from obvious deallocate). Apart from this, everything else should be good.