Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add per process chi² estimators #1200

Merged
merged 2 commits into from
May 28, 2021
Merged

Add per process chi² estimators #1200

merged 2 commits into from
May 28, 2021

Conversation

Zaharid
Copy link
Contributor

@Zaharid Zaharid commented Apr 20, 2021

Make the big table group by process and add plots grouping
chi² and phi by process, in addition to the existing ones by experiment.

Closes #1198.

Make the big table group by process  and add plots grouping
chi² and phi by process, in addition to the existing ones by experiment.
@Zaharid Zaharid requested review from RosalynLP and wilsonmr April 20, 2021 13:09
@Zaharid
Copy link
Contributor Author

Zaharid commented Apr 20, 2021

@wilsonmr @siranipour any idea why I get something like this

image

it seems to know what the group should be (and groups by process at some point) but then it uses experiments for some reason.

@Zaharid
Copy link
Contributor Author

Zaharid commented Apr 20, 2021

The values I get for the two groupings are also different: this appears in the same report, to be compared with the picture above
image
so it may be some sort of labelling issue....

@RosalynLP
Copy link
Contributor

This latter one is because one fit has the experiments set to DEUTERON and the other has them as separate experiments - that's what I was alluding to in #1196. For the first one I'm not sure why that happened - I'll take a look.

@Zaharid
Copy link
Contributor Author

Zaharid commented Apr 20, 2021

FWIW this does what I expect...

fits:
    - 210226-n3fit-FD01

use_cuts: fromfit

DataGroups:
  - metadata_group: nnpdf31_process
  - metadata_group: experiment

template_text: |
    {@DataGroups plot_fits_groups_data_chi2@}

actions_:
    - report(main=True)

@siranipour
Copy link
Contributor

Will take a look into it for you sheriff

@Zaharid
Copy link
Contributor Author

Zaharid commented Apr 20, 2021

@wilsonmr

Could this have something to do?

dataspecs:
  # WARNING: do not blindly copy and paste this: it can overwrite the datasets
  # for any actions which rely on grouping datasets.
  - data_input:
      from_: fitinputcontext
    theoryid:
      from_: current
    pdf:
      from_: current
    fit:
      from_: current
    speclabel:
      from_: current

  # WARNING: do not blindly copy and paste this: it can overwrite the datasets
  # for any actions which rely on grouping datasets.
  - data_input:
      from_: fitinputcontext
    theoryid:
      from_: reference
    pdf:
      from_: reference
    fit:
      from_: reference
    speclabel:
      from_: reference

@wilsonmr
Copy link
Contributor

It looks like that sort of error. But it shouldn't be that because you're not using the dataspecs in these actions right?

@siranipour
Copy link
Contributor

Potentially, try rebasing on #1197, this is discussed there

@siranipour
Copy link
Contributor

There is however this

fits_groups = collect("groups_data", ("fits", "fitcontext",))

I tried it with multiple fits and it still seems to work tho

@Zaharid
Copy link
Contributor Author

Zaharid commented Apr 20, 2021

Here a minimal example that fails.

This works if I remove the first {@fits_groups@}. It is probably the same issue as in #1190.

fits:
    - 210222-n3fit-FD02
    - 210226-n3fit-FD01

use_cuts: fromfit

DataGroups:
  - metadata_group: nnpdf31_process
  - metadata_group: experiment

template_text: |
    {@fits_groups@}

    {@with DataGroups@}
    # {@processed_metadata_group@}
    {@fits_groups@}
    {@endwith@}

actions_:
    - report(main=True)

@Zaharid
Copy link
Contributor Author

Zaharid commented Apr 29, 2021

This appears to work with NNPDF/reportengine#47

@Zaharid
Copy link
Contributor Author

Zaharid commented May 13, 2021

Here the report:

https://vp.nnpdf.science/v3AVnFdwQGOS1mO4g1Pl1A==

@wilsonmr
Copy link
Contributor

wilsonmr commented May 13, 2021

Looks good

except I think there is a bug in the barplot implementation: https://vp.nnpdf.science/v3AVnFdwQGOS1mO4g1Pl1A==/figures/DataGroups1_plot_fits_groups_data_chi2.png

Surely one fit should have bars present for the individual deuteron experiments and the other should have a single bar for DEUTERON. I'm not sure if that was part of your point in #1200 (comment) but I think the single bar at DEUTERON should be green because it's the chi2 for 210222-n3fit-FD02

@wilsonmr
Copy link
Contributor

wilsonmr commented May 13, 2021

Ignore that, it's just not right..

(I missed that one fit was collider only and didn't read the labels on the plot carefully)

@Zaharid
Copy link
Contributor Author

Zaharid commented May 27, 2021

Could someone merge this please?

@wilsonmr
Copy link
Contributor

Can I just confirm: this action will only work with the newest reportengine and we want these plots in the paper. But the tag includes reportengine before the fix required for this because you wanted to be safe not sorry. So how does producing these plots for the paper align with the policy? Or is that relaxed for producing reports?

@Zaharid
Copy link
Contributor Author

Zaharid commented May 28, 2021

To quote from the policy as I wrote about it

  • Post processing tools (postfit) MUST be run within the environment. Further analysis tools (for example vp-comparefits) MAY use updated versions of the code.

This was in anticipation of people asking for updates in reports/plots for the paper, which would be unreasonable to repeat fits for.

@wilsonmr
Copy link
Contributor

ok good, just double checking

@wilsonmr wilsonmr merged commit d189219 into master May 28, 2021
@wilsonmr wilsonmr deleted the comparefitspergroup branch May 28, 2021 09:52
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.

Clustering of chi2 values per process (not per experiment) in comparefits
4 participants