Externalize software compensation parameters

[rete]

• Externalized input parameters in LCSoftwareCompensation class
  • SC weights
  • SC density bins
  • Last energy density bin
• Added an separate function to register SC function in LCContent

This has also to be updated in DDMarlinPandora

[webermat]

Would it be possible to allow the numbers of SC density bins be a steerable parameter instead of the fixed 10 bins? In case we want to apply software compensation for large centre-of-mass energies the energy densities larger than e.g. 30 might play a more prominent role. Then we could extend the reach by adding more bins, while keeping the good performance using 10 bins for the current energy densities up to 30. This might proof be useful if we want to extend the application of software compensation for cluster energies beyond current default energy limit of 100 GeV.

For 3 TeV collisions the rate of neutral hadrons beyond 100 GeV increased from roughly 1.5 % for 500 GeV collisions to 13 % of the total neutral hadron energy spectrum.

[rete]

I agree with that we can change the number of possible bins to take that into account. Nevertheless, I think this can be implemented in a new PR. Here, I just tried to externalized the parameters for a better configuration through Marlin steering file parameters without modifying the original logic. Also, by changing the binning, you must also re-calibrate the weights, and then, the defaults weights have no sense anymore.

[rete]

Sorry, I realized that the check on the size of the bin array was not here before. I removed this check now

[PandoraPFA]

I would tend to agree with @webermat that it would make sense to allow specification of the number of energy bins too at this point. I would say that this step should indeed appear as a (series of) separate commit(s), but that it's arguably part of the same feature.

In that way, what would need to be enforced is that the number of bin edges be one greater than the number of weights, and that an overflow (final bin) weight be provided. Anyone changing the number of bins would then be forced to reevaluate the weights too, and it removes/improves the odd-looking:

https://github.com/PandoraPFA/LCContent/pull/19/files#diff-3ed3d72553079f38f8ef0e8bf37737acR27

What do you think?

[rete]

• A commit was added in the meantime to remove the check on the number of bins, so one can specify any bin edge array
• I don't understand why the number of bins should be equal to the number of weights + 1 ? A priori you can choose your binning as you want. Changing would of course requires a new calibration
• The last bin energy has a default value, so it is always defined
• Any any case, I think that changing any value of these parameters (bins or weights) requires a deep study of the calorimeters. The users playing with these value are normally aware of what they are doing.

To me the checks performed in this version seems OK.

[PandoraPFA]

Isn't the idea that the weights and binning have to match up, so that the output makes any sense? This means that there needs to be one more bin edge than there are weights to populate the bins. So one discrete bin (plus overflow) needs two edges, two discrete bins (plus overflow) needs three edges, etc.

A user should thus always provide the two vectors (plus an overflow value) together, using a set of weights that have been trained for the given binning. The default value is, obviously, exactly equivalent to a user providing a set of values, which just so happens to have 9 weights, with 10 bin edges and 1 overflow value.

Specific lines:
(pedantic) Prefer single line forward declaration for consistency with other usages:
e.g. namespace lc_content {class LCSoftwareCompensationParameters;}
https://github.com/PandoraPFA/LCContent/pull/19/files#diff-d99942265c6a5525494a74fef9f4500eR13

Trailing whitespace:
https://github.com/PandoraPFA/LCContent/pull/19/files#diff-8c080e80661f94ee318dfabef5892d7aR40

As per discussion above:
https://github.com/PandoraPFA/LCContent/pull/19/files#diff-3ed3d72553079f38f8ef0e8bf37737acR27

[StevenGreen1]

Hi John, Remi,

I believe that in my ill state I may have supplied John with false information earlier today. The number of weights is indeed just a parametrisation of the energy density function and the binning of that function is arbitrary, so there is no need for the number of weights to be equal to the number of bins + 1. The weights don't have a 1 to 1 correspondence to the bins, but they parametrise the function that determines each bins weight.

I also agree with @webermat in that it would be really good to make all these parameters (the weights, the bin edges and the overflow bin value) configurable via DDMarlinPandora. The changes to the LCContent for this PR and the fully configurable equivalent PR would be quite similar so it would be nice to have them incorporated together if that's ok with @rete.

Cheers

Steve

[rete]

I agree with @StevenGreen1 . Last remark : the weights array can't have a size different from 9 as it relies on the formula itself and thus MUST have a size of 9 :

https://github.com/rete/LCContent/blob/a3e6fe772b8ba581deca306ec21ee6458724e1ca/src/LCPlugins/LCSoftwareCompensation.cc#L158

Changing this size would mean changing the formula itself, thus the implementation.
With all the remarks and by seeing the current code, I think no modification is required.
The PR could be merged if you have no other remark or request.

Thank you all for your remarks !

Cheers, Remi

[StevenGreen1]

Hi Remi,

I was looking over the pull request this morning and would like to ask you if you could include the m_maxClusterEnergyToApplySoftComp, m_minCleanHitEnergyFraction, m_minCleanHitEnergyFraction and m_minCleanCorrectedHitEnergy variables in your external configuration also? Specifically, the m_maxClusterEnergyToApplySoftComp variable will be particularly important for CLIC related studies into software compensation and it would be nice if they didn't have to modify the Pandora settings file just for this variable. The other CleanClusters related logic variables would be there for completion. Are you happy to make this change @rete?

Thanks

Steve

[rete]

Hi all,

sorry for the late reply. I will do this in the afternoon and push commits asap.

Remi

[rete]

Should we merge this PR ?

[gaede]

@PandoraPFA can this PR be merged ? We are preparing a new iLCSoft release and it would be nice to have this in. Could you then also please create PandoraPFA release tags ?

[StevenGreen1]

I've checked out Remi's branch, made a few minor alterations that I've been meaning to do and merged the branch into the LCContent master.

The minor changes I implemented were:

• Updating vector range-based for loops to use c++11 notation
• Remove parameter names referring to weights (now called parameters) to avoid confusion with references to weights in the rest of Pandora.
• Check the cell volume before dividing by it to avoid divide by zero error.

I've also created a new LCContent tag (v03-01-01) for this commit and updated the ChangeLog.txt and CMakeLists.txt files accordingly.

Cheers

Steve