Conversation
and probably making a mess...Committing everything and probably making a mess...Committing everything and probably making a mess...Committing everything and probably making a mess...Committing everything and probably making a mess...Committing everything and probably making a mess...Committing everything and probably making a mess...Committing everything and probably making a mess...
- Fix a sign in the unc. symmetrization for the postfit pdf grids maker - Fix a bug where the central value was incorrectly paired with other variations, leading to a mismatch for the assymetric uncertainties With these fixes the Rabbit and Grids-based plots agree very well
Make the postfit grids maker able to read from rabbit or from a more generic hdf5 format. Also implement a writer to make this simpler format. The idea is to have the code work in a way that is easier to share with theorists (or possibly the EWWG)
|
From my perspective it would be good to have this reviewed and merged in, unless there are strong preferences for me to break it off into a separate package. |
| scale = scale if scale != -1.0 else self.pdf_inflation_factor(pdfInfo) | ||
| scale = ( | ||
| scale | ||
| if scale != -1.0 |
There was a problem hiding this comment.
Even though this was already present in the code, if scale is never supposed to be given negative values by the user (-1.0 was just a default to distinguish it from a user-specified not-negative number), isn't it safer to check as scale > -1.0 rather != -1.0?
Sometimes I am scared of float numbers being propagated and casted from float/double and silently changed to a slightly different value because of floating-point precision.
| import hist | ||
| import lz4.frame | ||
| import numpy as np | ||
| import pandas as pd |
There was a problem hiding this comment.
Pandas is a heavy library and should be only lazy loaded. In this case I'm not sure if it is needed, see my comment below
| if symm_type not in ["quadratic", "average"]: | ||
| raise NotImplementedError(f"Symmetrization type {symm_type} not supported!") | ||
|
|
||
| if type(matrix) == pd.DataFrame: |
There was a problem hiding this comment.
Suggest:
if hasattr(matrix, "values") and not callable(matrix.values):
values = matrix.values
elif hasattr(matrix, "values") and callable(matrix.values):
values = matrix.values()
else:
values = matrix
| return values, errors | ||
|
|
||
|
|
||
| def read_pdf_vals_and_errors(flavor, Q_scale, x_range, pdf_sets): |
There was a problem hiding this comment.
in general variables should not start with capital letters
| logger.info(f"{k}: {v}") | ||
| elif args.infile.endswith(".hdf5"): | ||
| with h5py.File(args.infile, "r") as f: | ||
| if "command" in f.attrs: |
There was a problem hiding this comment.
Should this not go into base_io? Which files contain the "command" and how was it generated? Could it not be stored in the supported way using a "meta" dict?
| help="Make sparse tensor", | ||
| ) | ||
| parser.add_argument( | ||
| "--rabbit-input", |
There was a problem hiding this comment.
Our convention was to use camelCase for CLA, also below
|
|
||
| symHessian = pdfInfo["combine"] == "symHessian" | ||
| symmetrize = indata.metadata["meta_info"]["args"]["symmetrizePdfUnc"] | ||
| print(f"PDF symmetrization procedure: {symmetrize}") |
There was a problem hiding this comment.
logger is defined, use it?
| ) | ||
| parser.add_argument( | ||
| "-p", | ||
| "--pdf-name", |
There was a problem hiding this comment.
use camelCase, also below
| f.attrs["pdf_name"] = pdf_helper.pdf_name | ||
| f.attrs["pdf_scale"] = pdf_helper.pdf_scale | ||
| f.attrs["pdf_symm"] = pdf_helper.pdf_symm | ||
| f.attrs["command"] = meta_info.get("command", "") |
There was a problem hiding this comment.
I think if you put that all inside a "meta" dict it is compatible with the usual format
|
|
||
|
|
||
| class RabbitPostfitPdfHelper(PostfitPdfHelper): | ||
| def __init__(self, fitresult_path, args=None, pseudoData=None): |
There was a problem hiding this comment.
In general I think we should avoid to pass args into modules/functions/classes. In this case I think it's not really needed
3 participants
No description provided.