Make CLI plot script a proper Python file

[felixhekhorn]

Closes #249

given the constraints here #310 (comment), this is the best we can do - however, I still believe this is a step forward, because at least plot.py is now a genuine Python file. It also needs no longer (Rust) formatting, but just a plain find and replace.

@cschwan since you may know the answer straight away (in contrast to me): can you help me with:

pineappl/pineappl_cli/src/plot.rs

Lines 549 to 550 in 300f3c6

	// TODO: read plot.py and replace "# CLI_INSERT_CONFIG\n" with `config`
	// and "# CLI_INSERT_DATA\n" with `data`

also note that the plot script itself is badly documented, i.e. there is no documentation, just some matplotlib magic going on, which is hard to understand

I also ran ruff on it, which generates more changes

[felixhekhorn]

Actually, after a bit more thinking I believe we can do even more, i.e. almost what I want:

1. I suggest to split all the static stuff, i.e. the plot functions, away from pineappl_cli/src/plot.py into a file pineappl_py/something/plot.py
2. that new file should be statically included into the pineappl Python package (and thus the Python interface will have easy access to the plot functions; we can also provide some additional methods which generate the necessary data from a grid using the Python interface itself)
3. the CLI will copy that file verbatim into the output of pineappl plot

[cschwan]

There are a bit too many changes doing different things in here, which make reviewing and fixing the tests difficult. Could you please focus first on one goal, namely making plot.py a proper Python file? We can reformat and lint later on when we know it's working. Reformatting will also likely break the NNPDF pheno scripts in the paper repo.

[felixhekhorn]

There are a bit too many changes doing different things in here, which make reviewing and fixing the tests difficult. Could you please focus first on one goal, namely making plot.py a proper Python file? We can reformat and lint later on when we know it's working. Reformatting will also likely break the NNPDF pheno scripts in the paper repo.

mmmmm ... [grummel] ... the commits so far are the minimal changes I would say, i.e. they make plot.py a Python file and the only thing on top, which is one of the main motivation of doing this thing, is ruff ... I don't know how one would do less

also I want to largely iterate the Python file to make it easier to maintain, to ban all the globals, and to make the plot functions reusable. Actually I already started on the process, but now I don't know whether to commit (and whether my last hour was wasted ...)

[felixhekhorn]

So, I committed nevertheless (because I don't want to loose my work).

I would say: if you want to have a smaller PR break this PR at 51a5d53. Now that may still be difficult too review, but this is because the previous state was bad - I'm 100% confident that this PR improves the situation. Yes, even that change can and may break the pheno paper plots, but then we can wait until that paper is out to merge - or maybe this PR reaches a state first, where whatever hacks were needed so far there are no longer needed. If you wish we can put the commits after the one mentioned into a separate PR - where I want to change lots of things.

Speaking of questionable content: what exactly is the purpose of map_format_e_join_repeat_last? if this is just for getting simpler arguments for matplotlib then I will ban that function and replicate the behaviour on the actual spot (if that is needed - because I believe sometimes it is counteracted again on the Python side)

[Radonirinaunimi]

I agree that the previous version of the script was in a lousy state and I think that we should fix this once and for all here. Hence I believe it would be reasonable to expect a few changes. Skimming through I think @felixhekhorn's changes are going in the right direction and it seems that to make the tests pass it will suffice to "copy" the contents of plot.py into tests/plot.rs.

may break the pheno paper plots

AFAIA, the pheno paper is not relying on this at all.

PS: I will have to wait for this to resume #310.

[alecandido]

A very stupid and almost irrelevant improvement:

ax.set_ylim(0.0)

(just to avoid the bottom gap)

[felixhekhorn]

A very stupid and almost irrelevant improvement:

@alecandido Maybe this comment belongs to #309?

[alecandido]

@alecandido Maybe this comment belongs to #309?

Ah, yes, I confused two open tabs, sorry 😅

[cschwan]

Is there anything missing here? If not, I'll fix the tests and merge it into master.

[Radonirinaunimi]

Is there anything missing here? If not, I'll fix the tests and merge it into master.

Not as far as I know. Things LGTM.

[Radonirinaunimi]

@felixhekhorn Can you confirm?

[felixhekhorn]

I still want to copy the file to pineappl_py, but this can also be done later, if you want to merge this quickly (to not block other PRs) (as I'm not sure how quickly I can come back to this).

Then at a later stage, I'd still like to improve the script as a whole and actually now that I'm thinking about it - some part of that will need to take place together with the move, i.e. it is not trivial, so let's postpone ...

[Radonirinaunimi]

@cschwan In case you've not worked on this yet I've fixed the tests locally that I could push (if it helps).

[cschwan]

Yes, please do!

[cschwan]

Are there any immediate concerns that should be addressed? Otherwise I'm going to merge this into master.

[Radonirinaunimi]

Are there any immediate concerns that should be addressed? Otherwise I'm going to merge this into master.

This actually could be merged.