Patch setupRabbit.py operations

[lucalavezzo]

--select and --axlim are possibly overlapping, and inconsistent with the order of operations like projecting. The goal of this PR is to separate the two, and ensure that things aren't quietly failing.

[cippy]

As I commented in Mattermost, I think this change of --select that adds the sum keyword to keep or remove the axis after the operation is making --select equivalent to --axlim (at least in the default case), which slices but doesn't remove the axis.
If the issue you found is that with --fitvar x-y and --select z A B the selection on z was ineffective because the histograms were projected onto x-y summing the full z axis before it could be selected, then the issue is that we don't have an option to specify when an action has to take place before others.
In fact, in that case the correct usage should have been to call --fitvar x-y-z and then --select z A B, which would remove the axis (otherwise if you need to keep the axis we already have --axlim to do the very same and we don't need to modify the other existing options)

[cippy]

Since you are touching this part, can we take this occasion to rename sum_axes (which is only used in this and the next few lines) as sum_gen_axes or something to explicitly mark it as such? It is probably already quite clear from the right had side that it only affects gen axes, usually for the unfolding or alphaS, but if you did it would make this part of the code easier to understand. Other possible axes on which we might apply actions are managed elsewhere, for example through the globalAction.

[cippy]

In spite of my previous comment, I think it makes sense that you made this action act more generically, allowing the user to control whether the marginalisation of the axis should happen or not.
Unfortunately it modifies the previous default behaviour, but if this is properly advertised to all users I can accept it

[lucalavezzo]

Hi @cippy @davidwalter2 thank you for the discussions. I propose the following

• --axlim to select on fitvar axes, as-is right now
• change the syntax to --axlim AXIS LOW HIGH
• change --select to --presel to make it clear it acts only non fitvars
• enforce --axlim and --presel to run only on fitvars and not fitvars, respectively
• --presel acts before all projections, rebinnings
• hard fail on --presel if the axis isn't present

what do you think?

[cippy]

Hi @cippy @davidwalter2 thank you for the discussions. I propose the following

• --axlim to select on fitvar axes, as-is right now
  Ok

• change the syntax to --axlim AXIS LOW HIGH
  Ok (it was high time we did that :) )

• change --select to --presel to make it clear it acts only non fitvars
  Ok

• enforce --axlim and --presel to run only on fitvars and not fitvars, respectively
  I think it is ok. We were using --select (soon to be --presel) on the 4th uT axis, to integrate it from input histograms which have it, but effectively this implies that we don't use it as fitvar (indeed we were not specifying it in the corresponding option), so this distinction should make sense.

• --presel acts before all projections, rebinnings
  Ok in general. However, there is an important special case we had to deal with so far. In order to fit only negative uT, we still needed the positive bin to derive the nonprompt estimate, and then we would select the bin afterwards. We achieved it in a hardcoded way based on setting all bins of uT>0 to 0 to forcefully exclude them from the fit, but this is potentially ill-defined and not appropriate. The proper way would be an additional option to slice the histograms at the very end, before saving the histograms (I can try to do it in a new PR myself, though, one detail to deal with is that we need to carefully update and propagate the fit axes ranges to the stored metadata in the tensor_writer or it complains when saving everything)

• hard fail on --presel if the axis isn't present
  Yes, better to fail rather than ignoring, if the user is calling this options it means he expects the axis to be present, and since one can now use --presel also to slice without removing the axis it is better to get a hard fail

what do you think?
I need more time to go through the code but I agree with the ideas, and if the code works I am fine with the changes

[cippy]

I am not generally using this whole group, but I think its logic was to have the minimal grouping with no redundancy (i.e. without adding also groups of listed subgroups), to avoid that one tends to double-count the lines with impacts

[cippy]

This requires changing the submodules I think, the current commit in the repository points to an older commit (CI didn't notice it, probably because it doesn't use it)

[cippy]

Fine to merge for me. Have you checked that the options work as expected? Especially for negative numbers, it might still be true that we need to pass " -2.4" (note the white space) instead of just -2.4 or "-2.4"