[WIP] Reintroducing cholesky

Created by: tgiani

Hi, here as discussed in the last pc I m trying to do the following:

1. reading the theory cov matrix from file at the level of ComputeCovMat_basic in chisquare.cc, using the function read_total_covmat (which btw should now be called read_theory_covmat)
2. construct the total cov matrix inside ComputeCovMat_basic, adding to the additive errors also the part coming from the theory cov mat, if present
3. performing the replica sampling doing a gaussian sampling using just the additive part of the total cov mat, and adding the mjultiplicative uncertainties by hand, like in the old implementation. The gaussian sampling is performed using cholesky decomposition
4. use the same function ComputeCovMat_basic to build the fit covariance matrix in Experiment::LoadFitCovMat

Created by: tgiani

Trying to run vp-setupfit using this pull request I m getting

[INFO]: All requirements processed and checked successfully. Executing actions.
[CRITICAL]: Bug in setup-fit ocurred. Please report it.
Traceback (most recent call last):
  File "/exports/csce/eddie/ph/groups/nnpdf/Users/tg/Documents/conda-nnpdf/nnpdf/validphys2/src/validphys/scripts/vp_setupfit.py", line 161, in run
    super().run()
  File "/exports/csce/eddie/ph/groups/nnpdf/Users/tg/Documents/conda-nnpdf/nnpdf/validphys2/src/validphys/app.py", line 141, in run
    super().run()
  File "/exports/csce/eddie/ph/groups/nnpdf/Users/tg/Documents/miniconda3/envs/dev/lib/python3.6/site-packages/reportengine/app.py", line 359, in run
    rb.execute_sequential()
  File "/exports/csce/eddie/ph/groups/nnpdf/Users/tg/Documents/miniconda3/envs/dev/lib/python3.6/site-packages/reportengine/resourcebuilder.py", line 165, in execute_sequential
    *self.resolve_callargs(callspec))
  File "/exports/csce/eddie/ph/groups/nnpdf/Users/tg/Documents/miniconda3/envs/dev/lib/python3.6/site-packages/reportengine/resourcebuilder.py", line 172, in get_result
    fres =  function(**kwdict)
  File "/exports/csce/eddie/ph/groups/nnpdf/Users/tg/Documents/conda-nnpdf/nnpdf/validphys2/src/validphys/results.py", line 385, in results
    return DataResult(data), ThPredictionsResult.from_convolution(pdf, dataset,
  File "/exports/csce/eddie/ph/groups/nnpdf/Users/tg/Documents/conda-nnpdf/nnpdf/validphys2/src/validphys/results.py", line 66, in __init__
    self._covmat = dataobj.get_covmat()
  File "/exports/csce/eddie/ph/groups/nnpdf/Users/tg/Documents/miniconda3/envs/dev/lib/python3.6/site-packages/NNPDF/nnpdf.py", line 4721, in get_covmat
    return _nnpdf.DataSet_get_covmat(self)
OSError: [experiments] error: Cannot read covariance matrix file from 

In this PR I have moved the implementation of read_total_covmat from experiment.cc to chisquare.cc. Could this affect the vp2 part?

Created by: Zaharid

@tgiani which config file are you using?

Created by: Zaharid

@voisey Actually can we have the closest we have to a fit candidate in nnpfcpp/config?

Created by: tgiani

@Zaharid I m using the first DIS-only runcard which was sent around

Created by: Zaharid

@voisey @tgiani could you please investigate this? Also please put some valid runcard in the repo to aid testing.

Created by: tgiani

@voisey @RosalynLP I don't know how the vp2 part of this works, I ve always touched only the cpp part. Do you have any idea of what could be the problem here? However if you can provide a DIS only runcard I will first try to run another test

Created by: voisey

Hi @tgiani, what exactly do you want the DIS only runcard to output? (Btw you want this to be a validphys runcard, right?)

Created by: tgiani

Hi @voisey, I m just thinking to a DIS only fit runcard with the th cov matrix, so that i can try to run a fit

Created by: voisey

I think you can get what you need from this comment: !244 (merged), but please let me know if not.

Created by: tgiani

I think the problem is in dataset.cc, where the function ComputeCovMat is called with the wrong number of inputs. I ve changed it and I m testing it now, will push some commits later

Created by: tgiani

This PR is not getting any error now, and it is ready to be reviewed. I ve tested it running a DIS only fit. The report comparing this fit with the baseline DIS only is https://vp.nnpdf.science/bt30WA5aRVqKzFZKlRWHLA==/ while the comparison with the previous bugged one is here https://vp.nnpdf.science/w3qAsWUTQ1u2jWEo-JLl3g==/

I m not sure if we are happy with the results or not. Even if the current fit looks better than the previous bugged one, it still has much bigger error band than the one without th cov mat..not sure what we expect to see actually

Created by: voisey

Thanks for this @tgiani! The new fit certainly looks better than the previous one, but I'm not sure exactly whether it's what we expect to see either. Thoughts @Zaharid, @scarrazza?

@tgiani, do you think now would be a good time to rename read_total_covmat as read_theory_covmat?

Created by: tgiani

@voisey yes sure, I ll do that

requested review from @enocera

Created by: scarrazza

Review: Approved

Merged by: scarrazza at 2019-01-16 16:03:41 UTC

closed

merged manually