Skip to content

Add python t0 covmat#1052

Merged
scarrazza merged 9 commits into
masterfrom
python_t0
Jan 27, 2021
Merged

Add python t0 covmat#1052
scarrazza merged 9 commits into
masterfrom
python_t0

Conversation

@wilsonmr
Copy link
Copy Markdown
Contributor

As discussed I'm adding one of the other ingredients for using the python commondata in fit. I've also added some actions so the new functions can be called from the API.

I thought I was being clever adding load_commondata to the CommonDataSpec class but it ended up making circular dependencies - I think a better option would have a provider which calls the funciton on a CommonDataSpec. Whilst I'm at it I will make a second provider which applies cuts.

@wilsonmr wilsonmr marked this pull request as draft January 14, 2021 13:08
@wilsonmr
Copy link
Copy Markdown
Contributor Author

cc: @siranipour no need to review yet but you may wnt to take a look - please bear in mind it still requires some work 😅

@wilsonmr wilsonmr changed the base branch from master to datakw_tests January 15, 2021 15:14
@wilsonmr wilsonmr marked this pull request as ready for review January 15, 2021 15:16
@wilsonmr
Copy link
Copy Markdown
Contributor Author

Should merge #1053 first. I just wanted the diff to only be relevant stuff. It occurs to me that we also need to be able to construct the weighted covmats, which I shall do in another PR.

Copy link
Copy Markdown
Contributor

@RosalynLP RosalynLP left a comment

Choose a reason for hiding this comment

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

@wilsonmr this is just my first look through it, I checked the tests and had a look at the structure and how everything works. It makes sense to me and looks good but I will go back and check things more thoroughly with numerical examples next week.

Comment thread validphys2/src/validphys/coredata.py Outdated
Comment thread validphys2/src/validphys/coredata.py Outdated
Comment thread validphys2/src/validphys/covmats.py Outdated
Comment thread validphys2/src/validphys/covmats.py Outdated
Comment thread validphys2/src/validphys/covmats.py Outdated
Comment thread validphys2/src/validphys/results_providers.py Outdated
Comment thread validphys2/src/validphys/results_providers.py Outdated
Comment thread validphys2/src/validphys/results_providers.py Outdated
@RosalynLP
Copy link
Copy Markdown
Contributor

RosalynLP commented Jan 18, 2021

@wilsonmr I was trying to use the API to play around with this a bit but I am running up against some problems... it's probably me doing something really stupid, but essentially I was trying to load a fit runcard and use that to find t0_covmat but it doesn't like the dataset_input in there, it wants sysnum for each dataset. I thought using the API might circumnavigate the intermediate dependencies but it looks like I am doing it wrong. Any help is much appreciated! (P.S. I cut out some of the output below because it was long.)

In [2]: from validphys.api import API                                                                                                                                                         

In [3]: import yaml                                                                                                                                                                           

In [4]: with open("9pt_runcard.yaml", "r") as stream: 
   ...:     input_dat = yaml.safe_load(stream) 
   ...:                                                                                                                                                                                       

In [5]: input_dat                                                                                                                                                                             
Out[5]: 
{'description': 'NNPDF3.1 baseline for global fit with 9 point theory covariance matrix',
 'dataset_input':  [{'dataset': 'NMCPD', 'frac': 0.5},
   {'dataset': 'NMC', 'frac': 0.5},
   {'dataset': 'SLACP', 'frac': 0.5},
   {'dataset': 'SLACD', 'frac': 0.5},
   {'dataset': 'BCDMSP', 'frac': 0.5},
   {'dataset': 'BCDMSD', 'frac': 0.5},
   {'dataset': 'CHORUSNU', 'frac': 0.5},
   {'dataset': 'CHORUSNB', 'frac': 0.5},
   {'dataset': 'NTVNUDMN', 'frac': 0.5},
   {'dataset': 'NTVNBDMN', 'frac': 0.5},
   {'dataset': 'HERACOMBNCEM', 'frac': 0.5},
   {'dataset': 'HERACOMBNCEP460', 'frac': 0.5},
   {'dataset': 'HERACOMBNCEP575', 'frac': 0.5},
   {'dataset': 'HERACOMBNCEP820', 'frac': 0.5},
   {'dataset': 'HERACOMBNCEP920', 'frac': 0.5},
   {'dataset': 'HERACOMBCCEM', 'frac': 0.5},
 ....

In [6]: API.t0_covmat(**input_dat)                                                                                                                                                            
Failed processing key dataset_input.
---------------------------------------------------------------------------
KeyError                                  Traceback (most recent call last)
~/nnpdf/validphys2/src/validphys/config.py in parse_dataset_input(self, dataset)
    299         try:
--> 300             name = dataset["dataset"]
    301             if not isinstance(name, str):

KeyError: 'dataset'
.......
ConfigError: 'dataset' must be a mapping with 'dataset' and 'sysnum'

@wilsonmr
Copy link
Copy Markdown
Contributor Author

That config isn't right, it should either be dataset_inputs (plural) or just be a single item. Since validphys sees dataset_input it tries to process that key, but then you provide a list instead of a mapping so it can't find dataset

@RosalynLP
Copy link
Copy Markdown
Contributor

That config isn't right, it should either be dataset_inputs (plural) or just be a single item. Since validphys sees dataset_input it tries to process that key, but then you provide a list instead of a mapping so it can't find dataset

Ah thank you, you are right of course!

Base automatically changed from datakw_tests to master January 19, 2021 14:13
This was referenced Jan 21, 2021
@wilsonmr
Copy link
Copy Markdown
Contributor Author

Just a bump to get a review for this

@siranipour
Copy link
Copy Markdown
Contributor

It's on my todo list chief, promise to take a look pronto

@RosalynLP
Copy link
Copy Markdown
Contributor

Yeah sorry same!

Copy link
Copy Markdown
Contributor

@siranipour siranipour left a comment

Choose a reason for hiding this comment

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

Looks great just some minor comments

Comment thread validphys2/src/validphys/results_providers.py Outdated
Comment thread validphys2/src/validphys/results_providers.py
Comment thread validphys2/src/validphys/results_providers.py
Comment thread validphys2/src/validphys/results_providers.py Outdated
Comment thread validphys2/src/validphys/tests/test_covmats.py
Comment thread validphys2/src/validphys/tests/test_covmats.py
Comment thread validphys2/src/validphys/tests/test_covmats.py
Comment on lines +261 to +265
central_values: None, np.array
1-D array containing alternative central values to combine with
multiplicative uncertainties. This array must have length equal
to :py:attr:`self.ndata`

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe we should specify the default value is None and if so the behaviour is to use the commondata's own experimental central values

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

So I looked up if this was easy to do in sphinx and it didn't really seem so, perhaps having the default assignment in the function arguments is enough

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oh I thought you meant just to add it to docstring. I'm not sure I understand your comment now. You mean like
def ...(..., central_values=self.central_values) or something? I think a default of None and explaining what happens is ok here. Perhaps I'm misunderstanding something.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh my bad, I've confused myself, I was referring to a comment I made in the review, but subsequently deleted. ignore that last comment.

Yeah if we just add default is None which uses the commondata's own central value to the docstring, that should befine

@scarrazza scarrazza merged commit 51c4934 into master Jan 27, 2021
@scarrazza scarrazza deleted the python_t0 branch January 27, 2021 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants