Covariance matrix dependency has become circular

Created by: wilsonmr

I want to note more thoroughly the cause behind #437 (closed)

When one runs a fit they call theory_covmat_custom which eventually depends on each_dataset_results_bytheory, since we are using the results tuple here then (thanks to my rushed PR #436) we would have to specify the new flag use_thcovmat_if_present: False which is kind of annoying. In the short term one can of course set the default for this to be False however I could see this later causing potential issues with reports.

In #427 I started to separate out the covariance matrix from the data and the theory. But in the most minor way. I think as we have spoken about many times we should look to have the covmat as an entirely different object. Providers should be able to take theory, results and covmat as seperate items and combine as appropriate. This would then, for example, mean that theory_covmat_custom would only depend on theory and results and so wouldn't require one to specify anything wrt the covmat. Obviously this would also break pretty much everything which currently uses the results tuple.

I was thinking that also the theorycovmat_config in the fit runcards should probably also be usable in a report setting, and that use_thcovmat_if_present should become some kind of option within that which is similar to use_cuts: fromfit. This could then offer the flexibility to calculate the covmat from scratch, which would be useful if one wanted to look at experiments not in the fit, with the theory covmat. It also I believe would make the entire theory covmat business in fits and reports a bit more unified and less confusing

I'm not quite sure how to proceed, I think extremely short term we should set use_thcovmat_if_present: False as default in config.py, I should document this somewhere as per @Zaharid's comment in #437 (closed). I think that the suggestion here and #427 are very closely related, however fixing every single providers which currently uses results or some collected version of it seems like a monstrous task: in #427 I only attempted to fix providers used in the report.

I guess we also want #427 out sooner rather than later since it is really about the formatting of the report and this issue concerns something a bit deeper about how validphys interacts with the covariance matrix

My suggestion is: Get #427 merged soon with the flag changed to be False and some better documentation so that running a fit with the current set of runcards works and the rvp-comparefits reports work (since the user specifies with vp-comparefit to use_thcovmat_if_present anyway and I think most people are using this interface to produce th covmat reports).

Have another PR which expands upon the seperation started by #427 of the data, theory and covmat and also unifies the theorycovmat_config to be the flag which works with reports and fit runcards (and document this change)

Created by: Zaharid

So I don't think the output of results needs to change at all. It is used everywhere in vp and it is a good abstraction for many of the things it is used for, such as data theory comparisons and chi²s. However we should change the input. Instead of computing anything in there, it should just put things together in the appropriate data structure. We should be moving towards separating data from theory and covariance matrices from everything else. Then we should have providers that group these things in different ways, e.g. to compute the theory covariance matrices.

IMHO one strength of the design of validphys is that providers are black boxes from the point of view of their outputs. You can completely change the internal structure and break them down in arbitrary ways without affecting anything. That also shows that one should prefer simpler data structures wherever possible, as these make it inherently easier to produce them in different ways. Contrast this with say the c++ code where everything is couples either explicitly or in practice.

As for how to do this in practice, sure, I'll have a look at #427. But I really thinm we have to move towards doing things properly. Mostly because the difficulty of doing that is not really bigger than keeping adding hacks.

Created by: Zaharid

As for what "doing things properly" means, I'd say one possible way is to reimplement most parts of libnnpdf in python. Probably the most difficult thing is #404 and it is mostly done. I would be happy if it could see some more progress, particularly around agreeing on the structure and benchmarking the convolution. We need something similar for commondata files #378 (closed). And then a decent way to construct covariance matrices (which I bet it would be faster than now with minimal effort).

Created by: wilsonmr

So I don't think the output of results needs to change at all. It is used everywhere in vp and it is a good abstraction for many of the things it is used for, such as data theory comparisons and chi²s

Fair enough, I guess for something like theory_covmat_custom though where AFAICT from the code it really just needs the theory prediction from each of the theoryids then it seems superfluous to also be providing it with data. I guess in the case results combined three inputs then in theory one could also just collect over theory predicitions instead for something like that?

Just to be clear though, in the short term are you satisfied with the consequences of setting use_thcovmat_if_present to be False? If so I will do that when I fix the conflicts on that branch

Created by: Zaharid

Yes, the reason why results is the only thing that gives you the theory predictions, but also the data and the covariance matrix (and the monkey and the jungle https://www.johndcook.com/blog/2011/07/19/you-wanted-banana/) is that that is how the underlying DataSet class works.

On Wed, 24 Apr 2019, 09:40 wilsonmr, notifications@github.com wrote:

So I don't think the output of results needs to change at all. It is used everywhere in vp and it is a good abstraction for many of the things it is used for, such as data theory comparisons and chi²s

Fair enough, I guess for something like theory_covmat_custom though where AFAICT from the code it really just needs the theory prediction from each of the theoryids then it seems superfluous to also be providing it with data. I guess in the case results combined three inputs then in theory one could also just collect over theory predicitions instead for something like that?

Just to be clear though, in the short term are you satisfied with the consequences of setting use_thcovmat_if_present to be False? If so I will do that when I fix the conflicts on that branch

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub #441 (closed), or mute the thread https://github.com/notifications/unsubscribe-auth/ABLJWUQWPFMT4RGVG2SZLK3PSAFFPANCNFSM4HHW3GHA .

closed