Group datasets by experiment PLOTTING label by wilsonmr · Pull Request #427 · NNPDF/nnpdf

wilsonmr · 2019-04-10T16:58:06Z

closes #413, closes #418

closes #174 (the dataspecs plots already exists)

At the moment this is EXTREMELY basic, but it explains better my approximate approach to addressing the problem that the plots by experiment in vp reports are essentially redundant.

Instead of rewriting the various actions (another possibility which shouldn't be discounted/forgotten) I aimed to keep the idea of experiment as some arbitrary combination of datasets. There is a production rule which for a given fit, looks at the plotting info and then groups those datasets into experiments. Then one can do something like

{@with experiments_from_plotting@}
{@plot_experiments_chi2@}
{@endwith@}

and the plot will have bars according to the plotting experiments. I then added a provider fixup_fitthcovmat which handles the loading of the fitthcovmat and fixes the experiment headers to match those of the experiments. Then anything which used fitthcovmat now uses fixup_fitthcovmat and in DataResult I now take the cross section of fixup_fitthcovmat for either experiment or dataset.

The final step would be introducing some minimal #356 so that one can calculate total chi2s required for some tables and the summarise_fits action. All of this is currently a bit gross at the moment, can probably be done in a cleaner way, or in fact I could stop with this approach and try something completely different, it has however produced this: (results include th covmat and may or may not be correct - they look rather sensible compared to not using th covmat but that's not very rigorous)

wilsonmr · 2019-04-10T17:00:03Z

urm probably I can do this better I was rushing to get it to work but I noticed that at the moment one cannot set use_theorycovmat: False since it isn't handled in logic

We should rethink the whole thing.

Zaharid · 2019-04-10T17:15:54Z

A great addition to reportengine.Config would be something that performs these two lines by itself.

Zaharid · 2019-04-10T17:18:53Z

We should rethink the whole thing.

Zaharid · 2019-04-10T17:20:15Z

This needs to have the new behaviour documented.

Zaharid · 2019-04-10T17:23:47Z

I think we should move towards having (data, theory predictions, covmat) -> chi2 instead of (results) -> chi2. Now covmat requires way too many switches (and mixing data and theory in DataSet has always been a big problem in libnnpdf).

Zaharid · 2019-04-11T10:44:08Z

@scarrazza @tgiani while I am unhappy with vp getting a lot uglier due to this whole covmat thing I don't see much to do here short of rewriting the whole thing (which we should be doing by now). Please do look and test this.

Zaharid · 2019-04-11T10:47:05Z

@wilsonmr Please rebase on top of master and force push to get the tests to work. Would be good to add tests with the theory covmat.

I am also thinking that if we are going to use a theory covmat everywhere, it should be computed by vp rather than loaded somewhere with potentially incompatible data. And in the case where it is loaded, we should be able to check that e.g. the cuts match. One way to go about that is storing the corresponding namespace information on the cuts, in line with #224.

Zaharid · 2019-04-11T10:47:32Z

In any case we need a plan on how this should look like medium term.

wilsonmr · 2019-04-11T10:52:51Z

sure, the loading is a bit problematic. The only possible problem is if it is to be computed then basically every report from now on will require up to 9 theories to run. Like you say, at the very least the loaded one should check that it's consistent with the cuts.

There is still more to do with this if it is the direction you want to go for now - this will break the total chi2 values everywhere because they will miss theory correlations between experiments (as defined in this branch)

wilsonmr · 2019-04-12T10:00:31Z

Last Commit:

Ok some decisions made here:

use_theorycovmat should only be a bool, the string option made it messy and was too prone to error
since experiments in the global namespace couldn't be overridden, I moved it to dataspecs specifically so it didn't interact with the production rules
removed the plot_pdfs_phi since it needed to be sorted anyway closes need plot_phi_dataspecs #174 (the dataspecs plots already exists) I also added a phi table, and so changed phi_data also returns Npoints
if theory covmat is to be used then I always calculate covmat and sqrt covmat 'externally' in the sense that I get the sqrt from numpy function, but this seems consistent with the so called internal function GetSqrtFitCovMat in fact I need to delete this function.

Finally here is a new vp-comparefits report https://vp.nnpdf.science/T9J7ouncRqK8WJY4avGGmA==

(compare to its old version https://vp.nnpdf.science/WfioqoYSSl2DCFyr1xgEsw==/#chi2-by-experiment the datasets are sometimes ordered differently but they seem to have same numbers, less/no NaNs and the new experiment plots look sensible)

To do: (provided this passes tests)

Add covmat checks
Clean up some redundant providers/ change documentation more thoroughly
add covmat tests
add to vp-comparefits report to indicate what covmat is used
The table fits_chi2_table total is still broken, calc total in more sensible way.

Zaharid · 2019-04-12T10:06:53Z

Can we add to vp-comparefits some info on what covmat is used for the various plots? This could be a piece of text after the title or some label in the plot.

wilsonmr · 2019-04-12T10:12:20Z

I can, but they should be the same in this case, just in a different order since the fits_chi2_datasets_table is grouped according the plotting label on the experiment index

Zaharid · 2019-04-12T10:12:43Z

Yeah, sorry just realized.

wilsonmr · 2019-04-12T10:26:36Z

oh also I'm very bad at thinking of proper variable names, so probably these need to be changed <- input appreciated

Zaharid · 2019-04-12T11:01:39Z

Yeah, I am not a fan of the name plot_fits_exp_by_plotting_chi2.

Zaharid · 2019-04-12T11:02:20Z

Specifically I don't think the names should contain references to plotting.

wilsonmr · 2019-04-12T11:18:44Z

I just wanted to have some kind of distinction between these experiments and the fitted experiment, the latter of which has some kind of meaningful bearing on the total chi2. I guess I need to change the tests to account for the new DataResult layout. I didn't quite do what you said wrt data, theory predictions, covmat -> chi2 here but vaguely moved towards it, where DataResult is a container of data and covmat and result is a container of that and the theory prediction

wilsonmr · 2019-04-12T11:25:12Z

As soon as #356 becomes a thing then this becomes less confusing, perhaps it would be sufficient to call the new providers *_experiments_* whatever and then on any provider used in the report with the old meaning of experiments, put a comment and a warning that the provider needs to be updated with data when it exists?

and delete for example plot_fits_experiments_chi2 which is either the same as plot_fits_exp_by_plotting_chi2 for old fits or is useless for newer fits...

wilsonmr · 2019-04-12T13:35:39Z

So this is a bit cleaner and at least some of the tests are running now, however they're also giving the wrong numbers, which I'm really confused by because the method shouldn't have been changed. I will have to look at this later as I'm away this weekend

wilsonmr · 2019-04-12T15:20:56Z

OK I was being an idiot and didn't allow for using t0set in covariance_matrix

Zaharid · 2019-04-15T12:20:04Z

@wilsonmr what is the status here? Should this be reviewed?

wilsonmr · 2019-04-15T16:09:06Z

I think there's a couple of things to do first. I will take a look now

wilsonmr · 2019-04-15T16:40:22Z

Ok I updated the report above with a new label to say if theory covmat was used (https://vp.nnpdf.science/T9J7ouncRqK8WJY4avGGmA==) is this ok? I went for the 'exp + th' label since it appears in the legends of the relevant plots and columns of tables now and I didn't want to make the strings unneccesarily long

Zaharid · 2019-04-15T16:43:44Z

It's fine, but I think we should have this info somewhere in the report in text form.

wilsonmr · 2019-04-15T16:48:19Z

Ok I'll also add that

Zaharid · 2019-04-26T13:11:37Z

I agree, in terms of documentation where is the best place for that? I don't remember the conclusion the other day whether to still use vp2 documentation or the new sphinx docs

I don't think we decided anything in particular. I'd say for now let's put it in the guide, as it is currently more helpful for users and the added cost of moving it later is negligible.

wilsonmr · 2019-04-26T13:25:44Z

If I do something like this:

dataset_input: {dataset: HERAF2CHARM}


dataspecs:
    - use_thcovmat_if_present: True
    - use_thcovmat_if_present: False

normalize_to: 0

use_cuts: "fromfit"

fit: 190310-tg-nlo-global-7pts

pdf:
    from_: fit

theory:
    from_: fit

theoryid:
    from_: theory

template_text: |
    {@plot_fancy_dataspecs@}
    {@dataspecs dataset_chi2_table@}

actions_:
    - report(main=True)

I see different values in the tables, but exactly the same error bars in the plots. Any idea why?

Well the errorbar of the theory predictions should be the same - it's just the spread of replicas, the issue is:

#For now, simply take the first data result. We'll need to improve this.
    results = [dataspecs_results[0][0], *[r[1] for r in dataspecs_results]]

in the two namespaces the data actually have different errorbars and so probably this needs a slight rethink

Zaharid · 2019-04-26T13:36:39Z

Ah, right.

…

On Fri, 26 Apr 2019, 15:25 wilsonmr, ***@***.***> wrote: If I do something like this: dataset_input: {dataset: HERAF2CHARM} dataspecs: - use_thcovmat_if_present: True - use_thcovmat_if_present: False normalize_to: 0 use_cuts: "fromfit" fit: 190310-tg-nlo-global-7pts pdf: from_: fit theory: from_: fit theoryid: from_: theory template_text: | ***@***.***_fancy_dataspecs@} ***@***.*** dataset_chi2_table@}actions_: - report(main=True) I see different values in the tables, but exactly the same error bars in the plots. Any idea why? Well the errorbar of the theory predictions should be the same - it's just the spread of replicas, the issue is: #For now, simply take the first data result. We'll need to improve this. results = [dataspecs_results[0][0], *[r[1] for r in dataspecs_results]] in the two namespaces the data actually have different errorbars and so probably this needs a slight rethink — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#427 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABLJWUTBEMPD63OXKWXAOJTPSL7FRANCNFSM4HE7HX7A> .

Zaharid · 2019-04-26T14:50:43Z

Can we make fitthcovmat None rather than False? Also use a different variable name inside the production rule.

…ble names, outputs either None or a ThCovMatSpec

adapted from https://www.wiki.ed.ac.uk/download/attachments/58984316/Theory_covariance_module_documentation.pdf?version=1&modificationDate=1553184077000&api=v2

wilsonmr · 2019-04-26T16:49:07Z

Ok I made the changes and added documentation.

Could somebody double check the documentation? I think I got the flags right for running fits based upon https://www.wiki.ed.ac.uk/download/attachments/58984316/Theory_covariance_module_documentation.pdf?version=1&modificationDate=1553184077000&api=v2 however the 7 points stuff wasn't entirely clear to me @voisey

I haven't checked if the second to last commit broke anything because my laptop is a bit full right now and I wanted to make sure that vp-setupfit etc. is all good now, I'll try and check at some point this weekend.

wilsonmr · 2019-04-29T10:49:36Z

ugh there were a few things broken, just sorting it out

…l fixes

wilsonmr · 2019-04-29T14:29:56Z

Before last commit the vp-comparefits command line flags didn't work - I thought they did.

Now the config production rule reads a bit clearer I think - unfortunately the logic still has to be a bit ugly to handle all cases. vp-setupfit works fine without needing use_thcovmat_if_present: False. All different options for vp-comparefits work (command line and interactive) and are documented

wilsonmr · 2019-04-29T14:30:44Z

+            dest='thcovmat_if_present',
+            action='store_false',
+            help="Do not use theory cov mat for calculating statistical estimators.")
+        parser.set_defaults(thcovmat_if_present=None)


User must specify to use or not on command line (or use interactive)

(before I had something that was like --th_covmat <True/False> but argparse reads it as string, this is probs best way to handle a boolean)

Also the default value being True or False doesn't play nicely with interactive mode

I am fine with not having a default for now. It is the kind of thing that people in the future will say is obvious one way or another, but it is difficult to say which way.

wilsonmr · 2019-04-29T14:31:49Z

+        argnames = (
+            'base_fit', 'reference_fit', 'title', 'author', 'keywords')
+        boolnames = (
+            'thcovmat_if_present',)


this is a bit dirty and looks redundant at the moment but I figured that perhaps we might want more booleans that can also be used with the interactive mode in the future and it makes it easy to see what's happening

voisey · 2019-04-29T15:06:23Z

Ok I made the changes and added documentation.

Could somebody double check the documentation? I think I got the flags right for running fits based upon https://www.wiki.ed.ac.uk/download/attachments/58984316/Theory_covariance_module_documentation.pdf?version=1&modificationDate=1553184077000&api=v2 however the 7 points stuff wasn't entirely clear to me @voisey

I haven't checked if the second to last commit broke anything because my laptop is a bit full right now and I wanted to make sure that vp-setupfit etc. is all good now, I'll try and check at some point this weekend.

I've just read through the documentation and it looks good to me. I haven't tested the flags but the 7-points stuff looks correct to me.

wilsonmr · 2019-04-29T15:08:37Z

awesome, thanks

Zaharid · 2019-05-01T16:39:57Z

Should I look at this then?

wilsonmr · 2019-05-02T09:35:01Z

Yes please

Zaharid · 2019-05-02T09:41:22Z

+
+##### 3-point
+
+```


Please use ```yaml for runcard examples.

wilsonmr · 2019-05-03T14:34:39Z

@Zaharid asides from the documentation which I can fix shortly, had you managed to look at the other files.. In particular I think you might be able to see a better way of going about the vp_comparefits.py changes

Zaharid · 2019-05-03T17:06:00Z

+
+Once the user has correctly specified the `theoryids` and additional flags for their chosen
+prescription then the user must specify which PDF will be used to generate the theory 'points' required
+to construct the theory covariance matrix. The user must additionally specify where the theory covariance is to be used. The theory


Please format the text to be 80 characters per line.

done, I thought 100 was fine nowadays? Either way I think some of the lines were well over.

Zaharid · 2019-05-03T17:11:40Z

+
+---------------------------------------------------------------------
+
+##### 3-point


We definitively need to make it so that this information is stored somewhere and one does not need to duplicate it all the time.

Zaharid · 2019-05-03T17:29:46Z

+`use_thcovmat_if_present: True` must have been fitted in the corresponding `fit`. If the
+corresponding fit has `use_thcovmat_if_present: False` then the user will be warned and there will
+be no contribution from the theory covariance matrix used in calculating statistical estimators for
+that runcard.


This seems like it should be an error, not a warning.

This behaviour makes all of the reports which compare fit with theory uncertainties to baseline fit possible. I don't think this should be an error especially since the flag is *_if_present.

Now the warning I think is fine however I noticed that for some fits the error gets printed loads of times (perhaps because I was in parallel mode?) is this expected? It seems unneccessary to have the warning appear like 30 times..

Zaharid · 2019-05-03T17:30:28Z

+be no contribution from the theory covariance matrix used in calculating statistical estimators for
+that runcard.
+
+Finally, the `use_thcovmat_if_present` flag can be specified at runtime when using the


I am not sure what "at runtime" means here.

Zaharid · 2019-05-03T17:33:09Z

-        must manually set `use_thcovmat_if_present` to be False, or provide an appropriate fit.
+        theory covariance matrix then returns `False`.
        """
        if not isinstance(use_thcovmat_if_present, bool):


I think this is unreachable because you already specify the type in the function signature.

I appear to be able to reach it

Ah, indeed. I thought I had done about this than open an issue:

NNPDF/reportengine#15

Zaharid · 2019-05-03T17:36:52Z

        """
        return label

+    def produce_experiments_from_plotting(self, fit):


I think this should be called something else. If it wasn't so late I would think about a better name... The main issue is that it is unclear what the input is from the name, and I would never have guessed that it is a fit.

Zaharid · 2019-05-03T17:44:37Z


    return df

+#TODO: Add check here that dataset appears in fitthcovmat (if true) and that cuts match


Don't we have a check that the cuts match below (it is not very good in that it doesn't actually look at the points but only at the totals, but...)

Yes, actually I think I meant to delete this

Zaharid · 2019-05-03T17:49:25Z

+            dest='thcovmat_if_present',
+            action='store_false',
+            help="Do not use theory cov mat for calculating statistical estimators.")
+        parser.set_defaults(thcovmat_if_present=None)


I am fine with not having a default for now. It is the kind of thing that people in the future will say is obvious one way or another, but it is difficult to say which way.

Zaharid · 2019-05-03T17:52:27Z

Sorry this took a while but it has been a long week. I now more or less seem the logic and find it ok given what we have. But it does add many things that have to be improved later.

…tion

wilsonmr · 2019-05-07T11:45:30Z

so I renamed the production rules to be like fits_data_groupby_experiment which I think does a better job of explaining the input and the output. Also addressed the formatting of the docs and some wording

Zaharid · 2019-05-07T16:55:22Z

This looks pretty good. But we should start systematically addressing the various things that are fundamentally wrong, including but not limited to #356 and #25 (probably by writing the thing in python).

Zaharid · 2019-05-07T16:56:14Z

Thanks for figuring out all the non trivial changes!

wilsonmr commented Apr 10, 2019

View reviewed changes

Zaharid requested review from Zaharid, scarrazza and tgiani April 10, 2019 17:14

Zaharid reviewed Apr 10, 2019

View reviewed changes

wilsonmr force-pushed the expbyplotlabel branch from a76ec5a to 3af0be9 Compare April 12, 2019 09:49

wilsonmr force-pushed the expbyplotlabel branch from 3af0be9 to 85fbe06 Compare April 12, 2019 13:31

wilsonmr mentioned this pull request Apr 15, 2019

Cannot disable theory covmat in vp-comparefits #433

Closed

wilsonmr added 2 commits April 26, 2019 16:29

changed produce_fitthcovmat to be more readable including extra varia…

baf5865

…ble names, outputs either None or a ThCovMatSpec

added documentation for running fit and report with th covmat

1d2dc7a

adapted from https://www.wiki.ed.ac.uk/download/attachments/58984316/Theory_covariance_module_documentation.pdf?version=1&modificationDate=1553184077000&api=v2

updated guide, changed boolean flags for vp-comparefits to work, smal…

e7a9abb

…l fixes

wilsonmr commented Apr 29, 2019

View reviewed changes

Zaharid reviewed May 2, 2019

View reviewed changes

Comment thread doc/validphys2/guide.md Outdated

##### 3-point

```

Copy link
Copy Markdown

Contributor

Zaharid May 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use ```yaml for runcard examples.

Zaharid changed the title ~~[WIP] Group datasets by experiment PLOTTING label~~ Group datasets by experiment PLOTTING label May 3, 2019

Zaharid reviewed May 3, 2019

View reviewed changes

changed production rule names, small fixes to formatting of documenta…

a7ecf11

…tion

Zaharid merged commit 5b44c8d into master May 7, 2019

Zaharid deleted the expbyplotlabel branch May 7, 2019 16:59

wilsonmr mentioned this pull request May 16, 2019

Should validphys tests use new API #459

Closed

Zaharid mentioned this pull request May 21, 2019

adding diagonal/blockdiagonal functionalities, round 2 #467

Merged

wilsonmr mentioned this pull request Jul 19, 2019

added process type to PLOTTING file #492

Merged


		---------------------------------------------------------------------

		##### 3-point


		return df

		#TODO: Add check here that dataset appears in fitthcovmat (if true) and that cuts match

Conversation

wilsonmr commented Apr 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Zaharid commented Apr 10, 2019

Uh oh!

Zaharid commented Apr 11, 2019

Uh oh!

Zaharid commented Apr 11, 2019

Uh oh!

Zaharid commented Apr 11, 2019

Uh oh!

wilsonmr commented Apr 11, 2019

Uh oh!

wilsonmr commented Apr 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Last Commit:

To do: (provided this passes tests)

Uh oh!

Zaharid commented Apr 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wilsonmr commented Apr 12, 2019

Uh oh!

Zaharid commented Apr 12, 2019

Uh oh!

wilsonmr commented Apr 12, 2019

Uh oh!

Zaharid commented Apr 12, 2019

Uh oh!

Zaharid commented Apr 12, 2019

Uh oh!

wilsonmr commented Apr 12, 2019

Uh oh!

wilsonmr commented Apr 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wilsonmr commented Apr 12, 2019

Uh oh!

wilsonmr commented Apr 12, 2019

Uh oh!

Zaharid commented Apr 15, 2019

Uh oh!

wilsonmr commented Apr 15, 2019

Uh oh!

wilsonmr commented Apr 15, 2019

Uh oh!

Zaharid commented Apr 15, 2019

Uh oh!

wilsonmr commented Apr 15, 2019

Uh oh!

Zaharid commented Apr 26, 2019

Uh oh!

wilsonmr commented Apr 26, 2019

Uh oh!

Zaharid commented Apr 26, 2019 via email

Uh oh!

Zaharid commented Apr 26, 2019

Uh oh!

wilsonmr commented Apr 26, 2019

Uh oh!

wilsonmr commented Apr 29, 2019

Uh oh!

wilsonmr commented Apr 29, 2019

Uh oh!

wilsonmr Apr 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

wilsonmr commented Apr 10, 2019 •

edited

Loading

wilsonmr commented Apr 12, 2019 •

edited

Loading

Zaharid commented Apr 12, 2019 •

edited

Loading

wilsonmr commented Apr 12, 2019 •

edited

Loading

wilsonmr Apr 29, 2019 •

edited

Loading