Change ThPredictions to python predictions by scarlehoff · Pull Request #1430 · NNPDF/nnpdf

scarlehoff · 2021-10-11T09:25:14Z

Closes #1347. In practice (for the user) nothing changes afaik, one can continue using ThPredictionResult.from_convolution but now it will use the python convolutions module instead. Everything works just the same, only a few regression test change but these changes are in the third digit (the annoying part is that the picture in test_plot changes also in a few pixels which is enough for the test to fail, so I had to redo that as well).

I added a few checks to convolutions.py but on second thought I don't know if they are very useful or at all.

Note: I'm not touching https://github.com/NNPDF/nnpdf/blob/master/validphys2/src/validphys/chi2grids.py since that module gets completely changed in #1424 and these two PR don't overlap for any other action.

Zaharid · 2021-10-12T11:09:16Z

Overall looks fine, but would like to go over this slowly when I have time. As an aside, we should change how the providers in results.py work at some point later: there should be separate actions for getting data and theory predictions (where theory predictions don't need e.g. a covmat) and these should be combined in the results action.

siranipour

Looks good. I'm yet to play with it, so will get round to this soon. I'm a bit confused at the need for taking central_value as an explicit argument though

validphys2/src/validphys/results.py

scarlehoff · 2021-10-12T13:14:27Z

Overall looks fine, but would like to go over this slowly when I have time

No rush (no infinite tranquility either, but no rush). For the LHAPDFSet alternative I will probably have to build on top of this one but the rest should continue being independent of each other.

scarlehoff · 2021-10-14T08:20:01Z

Regarding the fact of doing central predictions and the envelope separately: this can be easily implemented but it needs to be done in #1433 since I can add a flag to grid_values to include or not central values (instead in this PR I would need to either modify convolution or libNNPDF to be then remove in #1433)

validphys2/src/validphys/convolution.py

scarlehoff · 2021-11-10T18:33:22Z

The central_value problem will be solved two PR after this one so this one can be accepted and merged if people is pleased with the rest (if people is not pleased then such information would also be appreciated)

Sorry if I'm being pushy but I would like to work on this while I still remember what I was doing.

Zaharid

I think we need to look into how hessian sets are supposed to work here.

validphys2/src/validphys/results.py

Zaharid · 2021-11-12T11:41:49Z

validphys2/src/validphys/results.py

+            all_centrals = []
+            for d in datasets:
+                all_preds.append(predictions(d, pdf))
+                all_centrals.append(central_predictions(d, pdf))


Just a note to mostly myself that we are supposed to be rid of this in some subsequent PR.

All I can do is to promise I will work on this as soon as the dust is settled on the rest so that it doesn't get forgotten.

scarlehoff · 2021-11-12T12:41:14Z

I think we need to look into how hessian sets are supposed to work here.

Maybe we should have hessian regression tests as well...

scarlehoff · 2021-11-15T10:42:08Z

Mm, by using stats_class the MC pdfs are broken instead.

Edit: because libNNPDF uses the "pandas default" for ddof https://pandas.pydata.org/docs/reference/api/pandas.DataFrame.std.html so I'll change MCStats to reflect that.

scarlehoff · 2021-11-15T11:07:05Z

In the last commit I've had to update the regression test but it passes for rtol=1e-4 so I think it is ok.

Zaharid · 2021-11-18T13:57:37Z

FWIW this is still using the cpp commondata, for e.g. results. I suppose we would want to change that later.

Zaharid · 2021-11-18T14:01:08Z

I would want to make sure that nothing catastrophic happens with the vp report, but other than that it looks fine (given the various discussions on future directions).

Zaharid · 2021-11-18T20:47:55Z

@scarlehoff the problem looks real.

The relevant part being

   File "/usr/share/miniconda/envs/nnpdfenv/lib/python3.9/site-packages/validphys/results.py", line 549, in abs_chi2_data
    chi2s = all_chi2(results)
  File "/usr/share/miniconda/envs/nnpdfenv/lib/python3.9/site-packages/validphys/calcutils.py", line 73, in all_chi2
    diffs = th_result._rawdata - data_result.central_value[:,np.newaxis]
IndexError: too many indices for array: array is 0-dimensional, but 1 were indexed

Zaharid

This seems to fail.

validphys2/src/validphys/results.py

Zaharid · 2021-11-25T13:55:36Z

@scarlehoff this error https://github.com/NNPDF/nnpdf/runs/4324437054?check_suite_focus=true seems genuine (and not obvious to be what it is by looking quickly at it).

scarlehoff · 2021-11-25T13:59:33Z

Is the random NaN problem that I am unable to reproduce. I have a computer with Ubuntu looping on the test but it never fails...

edit: sometimes I miss Travis' debug capabilities :P

github-actions · 2021-11-25T17:38:11Z

Greetings from your nice fit 🤖 !
I have good news for you, I just finished my tasks:

Fit Name: NNBOT-4471b3d8e-2021-11-25
Fit Report: https://vp.nnpdf.science/W0xFsqKNRtiViNO2DO_wlw==
Fit Data: https://data.nnpdf.science/fits/NNBOT-4471b3d8e-2021-11-25.tar.gz

Check the report carefully, and please buy me a ☕ , or better, a GPU 😉!

Co-authored-by: Zaharid <zk261@cam.ac.uk>

scarlehoff · 2021-11-29T10:28:13Z

Rebased. After a tortuous journey this should all pass and work fine now.

github-actions · 2021-11-29T14:05:17Z

Greetings from your nice fit 🤖 !
I have good news for you, I just finished my tasks:

Fit Name: NNBOT-561ae1507-2021-11-29
Fit Report: https://vp.nnpdf.science/KLtDT2DORrieZDiM6I4dNw==
Fit Data: https://data.nnpdf.science/fits/NNBOT-561ae1507-2021-11-29.tar.gz

Check the report carefully, and please buy me a ☕ , or better, a GPU 😉!

Zaharid

I have ran this with a few options, and also looked at it many times by now. Seems fine to me.

scarlehoff added the destroyingc++ label Oct 11, 2021

scarlehoff linked an issue Oct 11, 2021 that may be closed by this pull request

ThPredictionsResult should not use C++ #1347

Closed

Base automatically changed from use_internal_cuts_for_fkstuff to master October 11, 2021 13:08

scarlehoff mentioned this pull request Oct 11, 2021

Update all example resources #1432

Closed

scarlehoff marked this pull request as ready for review October 12, 2021 10:25

scarlehoff requested review from Zaharid and siranipour October 12, 2021 10:25

siranipour reviewed Oct 12, 2021

View reviewed changes

validphys2/src/validphys/results.py Outdated Show resolved Hide resolved

validphys2/src/validphys/results.py Outdated Show resolved Hide resolved

validphys2/src/validphys/results.py Show resolved Hide resolved

validphys2/src/validphys/results.py Outdated Show resolved Hide resolved

Zaharid reviewed Oct 19, 2021

View reviewed changes

validphys2/src/validphys/convolution.py Outdated Show resolved Hide resolved

scarlehoff force-pushed the removing_cpp_thpredictions branch from 4e8c31f to 0bad6c1 Compare November 1, 2021 20:43

Zaharid requested changes Nov 12, 2021

View reviewed changes

scarlehoff mentioned this pull request Nov 15, 2021

Add a regresion thprediction test for a hessian PDF #1469

Merged

scarlehoff force-pushed the removing_cpp_thpredictions branch from 003488d to 48d528b Compare November 15, 2021 11:06

scarlehoff changed the base branch from master to regression_test_thprediction_hessian November 17, 2021 15:22

Zaharid added Core changes enhancement New feature or request run-fit-bot Starts fit bot from a PR. labels Nov 18, 2021

Zaharid approved these changes Nov 18, 2021

View reviewed changes

Base automatically changed from regression_test_thprediction_hessian to master November 18, 2021 16:15

Zaharid requested changes Nov 18, 2021

View reviewed changes

validphys2/src/validphys/results.py Outdated Show resolved Hide resolved

scarlehoff mentioned this pull request Nov 25, 2021

np.exp() makes libNNPDF fail down the line #1476

Closed

scarlehoff mentioned this pull request Nov 26, 2021

Dispatching Python covariance matrices #1477

Merged

scarlehoff and others added 14 commits November 29, 2021 11:19

use predictions instead of libNNPDF

9a6cebd

add a explicit check

64b641f

typofix

e061482

remove deprecation notices

814ed76

update example runcards

e031fce

address review comments

c24da20

Update validphys2/src/validphys/convolution.py

aef8745

Co-authored-by: Zaharid <zk261@cam.ac.uk>

rebase master

8520fa3

remove unused check

7f42378

Update validphys2/src/validphys/results.py

6f0c37f

Co-authored-by: Zaharid <zk261@cam.ac.uk>

fix for hessian

dfc047c

"fix" for MCStats

febe199

update regressions

55ba26a

remove squeeze

ab2242d

scarlehoff force-pushed the removing_cpp_thpredictions branch from 35d845b to ab2242d Compare November 29, 2021 10:19

scarlehoff changed the base branch from master to dispatch_python November 29, 2021 10:19

scarlehoff added run-fit-bot Starts fit bot from a PR. and removed run-fit-bot Starts fit bot from a PR. labels Nov 29, 2021

scarlehoff marked this pull request as ready for review November 29, 2021 10:22

Base automatically changed from dispatch_python to master December 1, 2021 15:20

Zaharid approved these changes Dec 2, 2021

View reviewed changes

Zaharid merged commit 8a00917 into master Dec 2, 2021

Zaharid deleted the removing_cpp_thpredictions branch December 2, 2021 16:33

Conversation

scarlehoff commented Oct 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Zaharid commented Oct 12, 2021

Uh oh!

siranipour left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

scarlehoff commented Oct 12, 2021

Uh oh!

scarlehoff commented Oct 14, 2021

Uh oh!

Uh oh!

scarlehoff commented Nov 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Zaharid left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Zaharid Nov 12, 2021

Choose a reason for hiding this comment

Uh oh!

scarlehoff Nov 12, 2021

Choose a reason for hiding this comment

Uh oh!

scarlehoff commented Nov 12, 2021

Uh oh!

scarlehoff commented Nov 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

scarlehoff commented Nov 15, 2021

Uh oh!

Zaharid commented Nov 18, 2021

Uh oh!

Zaharid commented Nov 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Zaharid commented Nov 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Zaharid left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Zaharid commented Nov 25, 2021

Uh oh!

scarlehoff commented Nov 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Nov 25, 2021

Uh oh!

scarlehoff commented Nov 29, 2021

Uh oh!

github-actions bot commented Nov 29, 2021

Uh oh!

Zaharid left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

scarlehoff commented Oct 11, 2021 •

edited

Loading

scarlehoff commented Nov 10, 2021 •

edited

Loading

scarlehoff commented Nov 15, 2021 •

edited

Loading

Zaharid commented Nov 18, 2021 •

edited

Loading

Zaharid commented Nov 18, 2021 •

edited

Loading

scarlehoff commented Nov 25, 2021 •

edited

Loading