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

Positivity cuts #2059

Merged
merged 21 commits into from
Jun 12, 2024
Merged

Positivity cuts #2059

merged 21 commits into from
Jun 12, 2024

Conversation

comane
Copy link
Member

@comane comane commented Apr 19, 2024

This PR addresses #2058

Main modifications:

  • removed load_commondata method from LagrangeSetSpec(DataSetSpec) so that it is directly inherited from DataSetSpec where cuts are loaded (before no cuts were loaded for LagrangeSetSpec type datasets)
  • For a LagrangeSetSpec promoted Cuts to InternalCutsWrapper so as to allow for the proper loading of cuts. --> note: rules need now to be passed to DataSetSpec
  • renamed kin of XGL and added new process for that positivity dataset
  • Changed the names of some INTEG and POS processes

REPORTS

@comane comane changed the title [WIP] Positivity cuts Positivity cuts May 12, 2024
@scarlehoff scarlehoff changed the base branch from master to hashable_rules May 13, 2024 16:10
@scarlehoff scarlehoff mentioned this pull request May 20, 2024
30 tasks
Base automatically changed from hashable_rules to master May 21, 2024 10:24
Copy link
Member

@scarlehoff scarlehoff left a comment

Choose a reason for hiding this comment

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

Thanks! Only some minor comments. Could you also link in this PR some of the fits you've done using this branch?

n3fit/runcards/examples/nnpdf40-like_pos_cut.yaml Outdated Show resolved Hide resolved
validphys2/src/validphys/core.py Show resolved Hide resolved
n3fit/runcards/examples/nnpdf40-like_pos_cut.yaml Outdated Show resolved Hide resolved
@@ -56,7 +56,7 @@ implemented_observables:
description: Deep Inelastic Scattering
label: 'positivity dataset: DIS $c+\bar{c}$ structure function $F_2^c$'
units: ''
process_type: DIS_F2C
process_type: POS_F2C
Copy link
Member

Choose a reason for hiding this comment

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

Why this change? The process is DIS_F2C, the only difference is that we use the dataset in a lagrange multiplier. Also, this change is not mirrored in process_options.py

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure about this one tbh, I think I thought that it might make more sense to have this process_type being under the POS ones.
What are you suggesting?

Copy link
Member

Choose a reason for hiding this comment

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

I think it is better to keep it as a POS_F2C. The positivity are special. I would however call them all POS_F2?
The other reason why I think they should not be called DIS_ is that there's a lot of hidden logic that treats DIS_ processes differently, better to keep the positivity ones differentiated.

Agree that a POS_F2 would be needed in process type (I guess you are already doing it like that but positivity/integrability datasets don't need kinematic coverage so no need to implement one).

Copy link
Member Author

Choose a reason for hiding this comment

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

Just to be sure, would you change all process types starting with POS_F2 with POS_F2 (eg. POS_F2C_CCP -> POS_F2)

What about other processes such as POS_FLL should we just leave them as they are?

Copy link
Member Author

Choose a reason for hiding this comment

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

I did rename all of the POS_F2* process types to POS_F2. I have also added the corresponding process types in process_options.py

Copy link
Member

@scarlehoff scarlehoff left a comment

Choose a reason for hiding this comment

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

Left some small comments, but other than that I think this is good to go.
Make sure you rebase the latests changes in master just to make sure nothing breaks but looks good to me.

n3fit/runcards/examples/Basic_runcard.yml Show resolved Hide resolved
@@ -56,7 +56,7 @@ implemented_observables:
description: Deep Inelastic Scattering
label: 'positivity dataset: DIS $c+\bar{c}$ structure function $F_2^c$'
units: ''
process_type: DIS_F2C
process_type: POS_F2C
Copy link
Member

Choose a reason for hiding this comment

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

I think it is better to keep it as a POS_F2C. The positivity are special. I would however call them all POS_F2?
The other reason why I think they should not be called DIS_ is that there's a lot of hidden logic that treats DIS_ processes differently, better to keep the positivity ones differentiated.

Agree that a POS_F2 would be needed in process type (I guess you are already doing it like that but positivity/integrability datasets don't need kinematic coverage so no need to implement one).

@comane comane merged commit 16c48f8 into master Jun 12, 2024
6 checks passed
@comane comane deleted the positivity_cuts branch June 12, 2024 16:10
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.

None yet

3 participants