Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add shape systematic #25

Merged
merged 15 commits into from
Nov 22, 2023
Merged

Conversation

dcookman
Copy link

A new systematic is added to OXO: Shape. The user can provide an arbitrary function that returns a value given a set of relevant observable values as well as any number of shape parameters (given in a ParameterDict). The effect of the systematic on a BinnedED is to scale each bin independently by the value returned by the user's function.

Because the bins get scaled independently, the response matrix generated with Construct() is diagonal. This class supports a user function that is only a function of a subset of the observable parameters.

Example use-case: in a solar oscillation analysis, the observed solar neutrino spectrum shape changes as a function of the solar oscillation parameters, solar flux, as well as the observed reconstructed energy of the event. A function can be developed by the user that describes the survival probability as a function of (energy, osc params, solar flux), and fed into a Shape systematic object. This can then be applied to an MC pdf (BinnedED object) to modify the shape accordingly, even if this pdf contains other observables that don't impact the survival probability: the event position, for example.

A critical aspect of this systematic is that the user might want it to impact the observed overall normalisation of a given pdf: in the above example, above just changing the shape of the energy spectrum, the total rate of events observed will drop significantly due to neutrino oscillations. By default of course, the usual functionality of asserting normalisation of the pdf(s) impacted by this systematic is maintained - this is how all other systematics currently function. If however the user wants to allow the normalisation to be impacted, then this default functionality can be turned off. In particular, The BinnedEDManager, SystematicManager, and BinnedNLLH classes have all been modified to handle this possibility. If you want to have the normalisation controlled by the systematic and not modified directly as a fit parameter directly from the BinnedEDManager within BinnedNLLH, then when you do BinnedNLLH::AddPdf(), there is now an extra option to turn off the standard normalisation handling.

In the existing OXO code, the normalisations of MC PDFs within the BinnedNLLH class have two related functions: one is to monitor the actual normalisation of the PDF, so that the NLLH can actually be calculated correctly, and the other is to be modified directly as a fit parameter. With the Shape systematic, by turning off the norm option when you add the PDF to the BinnedNLLH object the first property is maintained (otherwise the BinnedNLLH wouldn't be able to calculate likelihoods correctly!), but the latter is turned off. If the second property was kept on, then you end up with the problem of having fit parameters in both the BinnedEDManager AND SystematicManager trying to modify the normalisation, leading to chaos!

Going back to our solar example, by setting norm=false, there is no longer a FitParameter for directly modifying the normalisation of the oscillated solar spectrum. Instead, this normalisation can be indirectly modified by the systematic parameters: namely the solar flux and oscillation parameters! The actual normalisation value for the oscillated PDF after the systematic has been applied is still stored within the BinnedEDManager.

Even though I've mucked about with BinnedNLLH etc., hopefully functionality for existing code should be identical - if you never care about the shape systematic, then interacting with the BinnedNLLH class should be as before!

In addition to all of this, I have checked this new functionality with a new set of unit tests, found within ShapeTest.cpp. On top of the basic functionality, I also confirm that the interaction between the Shape systematic and the BinnedNLLH class is as I would want. This file also acts as kind of a tutorial, if someone wants to use this new systematic.

The code compiles, as well as the unit tests, and all the unit tests pass.

@willp240
Copy link

willp240 commented Jul 25, 2022

@dcookman l this is brilliant thanks a lot for implementing it! The last couple of days I've been having a proper play with it, more for my own understanding than doubting the code, but in doing so am very happy the code is doing as intended so will merge in soon.

I have one small but maybe slightly subtle question. Say I have two systematics being applied to one of my PDFs, say a shift then a shape, and I have the normalisation as false for this PDF (so the systematics control the normalisation). As I understand it as it stands, I would apply the shift, not renormalise, then apply the shape to the unnormalised PDF, and again not normalise. Whereas say I also have another PDF which only gets the shift systematic, I'd apply the shift and then renormalise. I'm sure it's only a small impact, but does it matter that the shift gets normalised for some PDFs but not others? Maybe it doesn't matter because the shape would change it anyway? I'm more thinking out loud but it would be useful to discuss. (and mat be fiddly to implement renormalising the same PDF after some systs but not others!)

Maybe this would be more relevant if you had say shift->shape->scale for some PDFs but just shift->scale for others. For the latter, the scale is being performed on a normalised PDF, but for the former the scale is being performed on an unnormalised PDF. But maybe that still doesn't matter? I need to think about it some more!

- constify SparseMatrix's Print methods
- Handle normalisation in NLLH calcs properly, via the NormFittingStatus, dealing with systematics & buffer bins properly now in all cases
- Major refactoring of Scale systematic, providing speed improvements of an order of magnitude!
@dcookman
Copy link
Author

@willp240 After a bunch more mucking about, I've made some further changes partly in response to your very good question. Here is a list of the additional changes I have made:

  • Const-ify the Print methods in SparseMatrix
  • Create a new enum, NormFittingStatus, which replaces the actions of the booleans stored within BinnedEDManager's fAllowNormsFittable vector. DIRECT acts like traditional OXO, having the normalisation of the PDF to be modified only by the fitter and not by any systematics or PDF shrinking; FALSE removes the ability for a given PDF to have its normalisation to be modified by the fitter (it can only be changed if there is some systematic that can modify that PDF's normalisation through a fitting parameter of its own); INDIRECT is the half-way house where the fitter can change the normalisation, but this is the normalisation before systematics or shrinking is applied.
  • I have made sure that in the FALSE and INDIRECT cases, the normalisation of a given PDF used in log-likelihood calculations is correctly calculated after any systematics and PDF shrinking is applied.
  • The Scale systematic is majorly refactored for much greater efficiency. (This frankly should have been its own PR, sorry!) I use the same approach seen in other systematics, of pre-calculating the bin mapping from the scaling subspace to the full space for the PDF upon which the systematic acts; calculating the response matrix over the scaling subspace first then converting into the full space via this mapping; moving the calculation of the actual bin contribution to a separate private method for clarity; and modifying this private method's algorithm to only consider bins that could plausibly have a contribution, and fix a long-standing bug in the contribution calculation.
  • Some further unit tests have been added.

@willp240 willp240 added the merge soon to be merged soon if no objections label Nov 22, 2023
@willp240 willp240 merged commit 94a91e3 into snoplus:master Nov 22, 2023
@willp240 willp240 mentioned this pull request Nov 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge soon to be merged soon if no objections
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants