Skip to content

Fix callers of group_chi2_table#1525

Merged
Zaharid merged 3 commits into
masterfrom
procs_fix
Feb 25, 2022
Merged

Fix callers of group_chi2_table#1525
Zaharid merged 3 commits into
masterfrom
procs_fix

Conversation

@Zaharid
Copy link
Copy Markdown
Contributor

@Zaharid Zaharid commented Feb 17, 2022

Commit 79604bd had introduced a level of indirection on the inputs of
groups_chi2_table to make sure that the data wasn't being reshuffled too
much. This however was not propagated to the actions that internally
made use of that functionality, of which there are three.

Fix the simpler one to use the original action with the right grouping
and add an additional collect over processes for the ones in the theory
covariance matrix module.

Closes #1520.

@Zaharid
Fix callers of group_chi2_table
c0e3ef3 
Commit 79604bd had introduced a level of indirection on the inputs of
groups_chi2_table to make sure that the data wasn't being reshuffled too
much. This however was not propagated to the actions that internally
made use of that functionality, of which there are three.

Fix the simpler one to use the original action with the right grouping
and add an additional collect over processes for the ones in the theory
covariance matrix module.
@Zaharid Zaharid added the bug Something isn't working label Feb 17, 2022
@andreab1997
Copy link
Copy Markdown
Contributor

andreab1997 commented Feb 18, 2022

I tried running the following runcard

meta:
   author: Andrea Barontini
   keywords: [theory uncertainties, 9-point]
   title: NLO 9-point variations for 1 process type - DIS CC

default_theory:
   - theoryid: 163

theoryid: 163
point_prescription: '9 point'

theoryids:
   from_: scale_variation_theories


dataspecs:
        - theoryid: 163
          speclabel: $(\xi_F,\xi_R)=(1,1)$
        - theoryid: 177
          speclabel: $(\xi_F,\xi_R)=(2,1)$
        - theoryid: 176
          speclabel: $(\xi_F,\xi_R)=(0.5,1)$
        - theoryid: 179
          speclabel: $(\xi_F,\xi_R)=(1,2)$
        - theoryid: 174
          speclabel: $(\xi_F,\xi_R)=(1,0.5)$
        - theoryid: 180
          speclabel: $(\xi_F,\xi_R)=(2,2)$
        - theoryid: 173
          speclabel: $(\xi_F,\xi_R)=(0.5,0.5)$
        - theoryid: 175
          speclabel: $(\xi_F,\xi_R)=(2,0.5)$
        - theoryid: 178
          speclabel: $(\xi_F,\xi_R)=(0.5,2)$

normalize_to: 1

fit: 190315_ern_nlo_central_163_global
use_cuts: "fromfit"

pdf:
    from_: fit


dataset_inputs:
  - dataset: CHORUSNU
  - dataset: CHORUSNB
  - dataset: NTVNUDMN
  - dataset: NTVNBDMN
  - dataset: HERACOMBCCEM
  - dataset: HERACOMBCCEP

template: template_matrix_plots.md

dataset_report:
   meta: Null
   template_text: |
      ## Scale variations as a function of the kinematics for {@dataset_name@}
      {@plot_fancy_dataspecs@}

actions_:
  - report(main=true)

and it does not result in an error anymore. Anyway it looks like that the two tables of the output report (Experimental chi2 by dataset and Total chi2 by dataset) are grouped in a different way (the first by experiment and the second by process) despite the fact that they are generated by the same function (groups_chi2_table). Looking into that I noticed that the function is actually called twice but in the first case groups_data has three entries corresponding to the experiments and in the second case it has only one entry corresponding to the only process (DIS CC). Is this a bug or am I missing an entry in the runcard? @Zaharid

@andreab1997
Copy link
Copy Markdown
Contributor

andreab1997 commented Feb 18, 2022

I tried running the following runcard

meta:
   author: Andrea Barontini
   keywords: [theory uncertainties, 9-point]
   title: NLO 9-point variations for 1 process type - DIS CC

default_theory:
   - theoryid: 163

theoryid: 163
point_prescription: '9 point'

theoryids:
   from_: scale_variation_theories


dataspecs:
        - theoryid: 163
          speclabel: $(\xi_F,\xi_R)=(1,1)$
        - theoryid: 177
          speclabel: $(\xi_F,\xi_R)=(2,1)$
        - theoryid: 176
          speclabel: $(\xi_F,\xi_R)=(0.5,1)$
        - theoryid: 179
          speclabel: $(\xi_F,\xi_R)=(1,2)$
        - theoryid: 174
          speclabel: $(\xi_F,\xi_R)=(1,0.5)$
        - theoryid: 180
          speclabel: $(\xi_F,\xi_R)=(2,2)$
        - theoryid: 173
          speclabel: $(\xi_F,\xi_R)=(0.5,0.5)$
        - theoryid: 175
          speclabel: $(\xi_F,\xi_R)=(2,0.5)$
        - theoryid: 178
          speclabel: $(\xi_F,\xi_R)=(0.5,2)$

normalize_to: 1

fit: 190315_ern_nlo_central_163_global
use_cuts: "fromfit"

pdf:
    from_: fit


dataset_inputs:
  - dataset: CHORUSNU
  - dataset: CHORUSNB
  - dataset: NTVNUDMN
  - dataset: NTVNBDMN
  - dataset: HERACOMBCCEM
  - dataset: HERACOMBCCEP

template: template_matrix_plots.md

dataset_report:
   meta: Null
   template_text: |
      ## Scale variations as a function of the kinematics for {@dataset_name@}
      {@plot_fancy_dataspecs@}

actions_:
  - report(main=true)

and it does not result in an error anymore. Anyway it looks like that the two tables of the output report (Experimental chi2 by dataset and Total chi2 by dataset) are grouped in a different way (the first by experiment and the second by process) despite the fact that they are generated by the same function (groups_chi2_table). Looking into that I noticed that the function is actually called twice but in the first case groups_data has three entries corresponding to the experiments and in the second case it has only one entry corresponding to the only process (DIS CC). Is this a bug or am I missing an entry in the runcard? @Zaharid

I found that this problem is related to these three collect

nnpdf/validphys2/src/validphys/results.py

Line 184 in 3fa10e6

groups_data = collect("data", ("group_dataset_inputs_by_metadata",))

nnpdf/validphys2/src/validphys/results.py

Line 663 in 3fa10e6

groups_each_dataset_chi2 = collect("each_dataset_chi2", ("group_dataset_inputs_by_metadata",))

nnpdf/validphys2/src/validphys/results.py

Line 803 in 3fa10e6

groups_chi2 = collect(

which groups data "by_metadata". However the theory+experimental table is done by the function

nnpdf/validphys2/src/validphys/theorycovariance/output.py

Line 27 in 3fa10e6

def procs_chi2_table_theory(

which calls groups_chi2_table with other inputs.
Changing "by_metadata" to "by_process" in the three collects above fixes the problem but I am not sure this is the right way of fixing it. @Zaharid

@Zaharid
Copy link
Copy Markdown
Contributor Author

Zaharid commented Feb 21, 2022

@andreab1997 It should work so that actions used elsewhere such as groups_data are not affected, for example by defining new collects by process. It would be good if you could do that, e.g. by pushing to this PR and test around.

@andreab1997
Copy link
Copy Markdown
Contributor

andreab1997 commented Feb 21, 2022

@andreab1997 It should work so that actions used elsewhere such as groups_data are not affected, for example by defining new collects by process. It would be good if you could do that, e.g. by pushing to this PR and test around.

@Zaharid yes, you're right. I've defined new collects for groups_data and groups_chi2 (but not for groups_each_dataset_chi2 because it is only used here).
I have also removed the group itself from the table because I believe it is misleading to have on the same level groups and datasets belonging to some group. I tested with the same runcard I posted before and it is working.

@scarlehoff
Copy link
Copy Markdown
Member

scarlehoff commented Feb 21, 2022

We have been looking at this functions and I wonder whether it doesn't make more sense to scratch them all and use what we use normally i.e., dataspecs_chi2_table, so that we don't have to play any tricks and the grouping is selected in the runcard with metadata_group?

@Zaharid
Copy link
Copy Markdown
Contributor Author

Zaharid commented Feb 24, 2022

Is this good to merge?

@scarlehoff
Copy link
Copy Markdown
Member

scarlehoff commented Feb 24, 2022

I would need to go through it, the server was down so I couldn't even run reports :_ I'll test it next week.

scarlehoff
scarlehoff approved these changes Feb 25, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@scarlehoff scarlehoff left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I think this is fine https://vp.nnpdf.science/UggLD0bKRouUZHnBOKsBfQ==/

other than the fact that the table header says group when it is actually datasets but that's not what this PR is fixing. Once it's merged we can open a new issue with that.

@Zaharid
Copy link
Copy Markdown
Contributor Author

Zaharid commented Feb 25, 2022

Great, thanks.

@Zaharid Zaharid merged commit 2eb86f2 into master Feb 25, 2022
@Zaharid Zaharid deleted the procs_fix branch February 25, 2022 12:28
@scarlehoff scarlehoff mentioned this pull request Feb 25, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@scarlehoff scarlehoff scarlehoff approved these changes

@andreab1997 andreab1997 Awaiting requested review from andreab1997

Assignees

No one assigned

Labels

bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

covariance_matrix_plot runcard fails when executing procs_chi2_table

3 participants

@Zaharid @andreab1997 @scarlehoff