Repair zero-scaled grids

[felixhekhorn]

#338 revealed a bug when multiplying a grid with 0 (i.e. default value), which got fixed by 591cdcf . However, this does not fix grids, which got multiplied before before that commit - which still yields problems, see NNPDF/pineko#228 .

The proposed solution is explained here

(I don't think I can do this on my own - I'm not an experienced enough PineAPPL developer myself, but I want to give an incentive for others to jump in 😇 )

I think iterating on all subgrids and call packed_array::clear_if_empty might be enough

[felixhekhorn]

Questions:

1. shouldn't --repair be an option of pineappl write? i.e. if I follow "never touch the input" I need a target and write is the place to be
2. should we use the already present and empty --upgrade?

[cschwan]

Questions:

1. shouldn't `--repair` be an option of `pineappl write`? i.e. if I follow "never touch the input" I need a target and `write` is the place to be

Yes, exactly.

2. should we use the already present and empty `--upgrade`?

No, --upgrade upgrades the internal data structures to newest available versions.

[cschwan]

Thank you @felixhekhorn, for starting this! I added the functionality in commit 5533f4c and the CLI switch in commit 829626d.

To finish the PR, do we need a Python wrapper to Grid::repair? I suppose we need to answer whether we want to unconditionally repair grids in Pineko (potentially expensive), then we need the wrapper, or we want to repair the grids on a case-by-case basis. The latter has the advantage that we always know when something is going wrong.

[Radonirinaunimi]

Thanks a lot @felixhekhorn and @cschwan for this! I had a quick look and everything seems fine to me.

To finish the PR, do we need a Python wrapper to Grid::repair? I suppose we need to answer whether we want to unconditionally repair grids in Pineko (potentially expensive), then we need the wrapper, or we want to repair the grids on a case-by-case basis. The latter has the advantage that we always know when something is going wrong.

I would say it would be good to have this regardless, especially if it turns out that many grids were affected by this issue. One can then have a try...except around optimize in Pineko.

[felixhekhorn]

I see you took a quite different approach @cschwan then myself, which is proof again how different people approach problems. I consider that a strength of development, because e.g. in this case your version is much shorter 👍

as for the API: I'm tempted to say, that repair should be an intrinsic part of optimize, because using the current file version is an optimization, i.e. Grid::optimize should call Grid::repair (potentially upon a optimization flag, which defaults to true). I think assuming that the current file version is better then the old is a safe one. The (potential) computational cost of repair and optimize are comparable, so we can join them.

[cschwan]

as for the API: I'm tempted to say, that repair should be an intrinsic part of optimize, because using the current file version is an optimization, i.e. Grid::optimize should call Grid::repair (potentially upon a optimization flag, which defaults to true). I think assuming that the current file version is better then the old is a safe one. The (potential) computational cost of repair and optimize are comparable, so we can join them.

I almost made it part of Grid::optimize, but now I wouldn't call it an optimization for the simple fact that Grid::repair prevents NNPDF/pineko#228 and it that sense it is a repair routine. I also spotted a potentially second operation that could become part of Grid::repair: the last open item of #358.

[Radonirinaunimi]

I would also prefer repair to be a separate method. And in that regard, I guess that would be the only piece missing before we can merge this, at least in order to propagate into pineko so that people can compute EKOs again.

[felixhekhorn]

I would also prefer repair to be a separate method. And in that regard, I guess that would be the only piece missing before we can merge this, at least in order to propagate into pineko so that people can compute EKOs again.

done in 1b27a25 (however I failed to come up with a quick unit test)

[cschwan]

LGTM. Merge whenever the tests have finished and you're happy.

[felixhekhorn]

No idea on what the problem is, you did merge master but he still refuses to "rebase and merge" and indeed on the console I had to fix a merge conflict. However, then I could merge this branch cleanly on top of master - see there. Still Github didn't realize this here, so I close this PR any ways ...