Option to draw hit markers with colour representing charge (calorimet…

[cvilelahep]

Veto/HCal/Muon system hit colour represents total QDC for that hit. SciFi hit colour represents number of hits within +- 0.5 cm of the drawn hit.

Example of muon candidate with hitColour = None:

Same event with hitColour = "q":

[olantwin]

I'll comment in detail tomorrow.

First impressions:

1. It's a lot of code with many calculations. It might be good to split it up slightly into funtions and possibly multiple commits (e.g. one for the colour options, one for the legend and one for the additional information display).
2. Could we use a colourbar as a legend instead of individual points?
3. I'll definitely have a few nitpicks about formatting and readability ;)
4. Is there a way to set this option via the commandline? It seems one needs to modify the python script to turn it on, unless I missed something while skimming?

[cvilelahep]

Hi @olantwin,

It's a lot of code with many calculations. It might be good to split it up slightly into funtions and possibly multiple commits (e.g. one for the colour options, one for the legend and one for the additional information display).

The calculation of the hit density should probably be done in the analysis SciFi Tools library. When that becomes available we can remove it from the event display code. There are no other calculations other than defining positions of text, etc. Sorry for bundling everything into a single commit. I don't have time to split this into several commits.

I'm not sure what is meant by "additional information display". This PR only adds colour to the markers and the legend required to interpret those colours.

Could we use a colourbar as a legend instead of individual points?

This would be my preference, but I couldn't find a way of doing it, and I don't have time to work on this any further. It may not be the prettiest solution, but it does the job.

Is there a way to set this option via the commandline? It seems one needs to modify the python script to turn it on, unless I missed something while skimming?

It would be trivial to change the code to grab this option from the CLI. However, the 2dEventDisplay script code is written in such a way that it pretty much has to be run in interactive mode (I use heredocs to get around this), so having it as an argument to loopEvents doesn't typically require modifying the script.

[olantwin]

It's a lot of code with many calculations. It might be good to split it up slightly into funtions and possibly multiple commits (e.g. one for the colour options, one for the legend and one for the additional information display).

The calculation of the hit density should probably be done in the analysis SciFi Tools library. When that becomes available we can remove it from the event display code.

For now, could it be split into a function? It would make following the code a bit easier.

There are no other calculations other than defining positions of text, etc. Sorry for bundling everything into a single commit. I don't have time to split this into several commits.

I'm not sure what is meant by "additional information display".

Sorry, my bad. I meant the purple text with QDC values etc. I don't use the 2dEventDisplay much, so naively assumed you were adding it.

This PR only adds colour to the markers and the legend required to interpret those colours.

Could we use a colourbar as a legend instead of individual points?

This would be my preference, but I couldn't find a way of doing it, and I don't have time to work on this any further. It may not be the prettiest solution, but it does the job.

Then let's maybe add an issue for that as a future improvement. Again, making a function for calculating the legend might make the code somewhat easier to digest.

Is there a way to set this option via the commandline? It seems one needs to modify the python script to turn it on, unless I missed something while skimming?

It would be trivial to change the code to grab this option from the CLI. However, the 2dEventDisplay script code is written in such a way that it pretty much has to be run in interactive mode (I use heredocs to get around this), so having it as an argument to loopEvents doesn't typically require modifying the script.

I see. What do you mean by heredoc in this context? May I suggest checking for q or charge as options? It might be also good to add a docstring with the hitColour option documented (we can add documentation for the other options later @siilieva ). This documentation should show up, when people run in ipython interactively.

[olantwin]

Appreciate the cleanup! Hopefully we can just move to automatically formatting code soon, so don't need to invest any work in making/keeping code readable.

[cvilelahep]

Factorised the hit colour code into functions. @olantwin, I think that's all of the comments that could be implemented for now.

[olantwin]

Could you please rebase? I think the merge you did caused some conflicts.

[cvilelahep]

Oops... should be ready to go now @olantwin

[olantwin]

@cvilelahep It would be great if you could avoid making merge commits next time. The history on main is a bit of a mess, with a lot of commits appearing twice or even thrice (well, it's more complicated than that. Essentially they appear once in each side of the history that was now merged).