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

Generate kfactors for polarized observables #132

Closed
wants to merge 11 commits into from

Conversation

toonhasenack
Copy link
Collaborator

Initial pull request for generating the K-factors of the F1 structure functions for polarized DIS data.

@giacomomagni giacomomagni added the enhancement New feature or request label Oct 23, 2023
@giacomomagni
Copy link
Contributor

This PR is mostly to keep track of @toonhasenack work, we might no merge this in the end.

@giacomomagni giacomomagni marked this pull request as draft October 23, 2023 08:52
@giacomomagni giacomomagni removed the enhancement New feature or request label Oct 23, 2023
@felixhekhorn felixhekhorn changed the title Generate kfactors Generate kfactors for F1 division Oct 23, 2023
@felixhekhorn felixhekhorn added the enhancement New feature or request label Oct 23, 2023
data/kfactors/generate.py Outdated Show resolved Hide resolved
import argparse


def get_gpaths(folder):
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider adding type hints

Copy link
Contributor

Choose a reason for hiding this comment

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

@toonhasenack when using type hints you can actually drop the repeated argument type in the doc string - see e.g. the second example here

strf_data += f"{data[i]} 0.0\n"

date = dt.now().date()
string = (
Copy link
Contributor

Choose a reason for hiding this comment

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

Make this a module constant

+ strf_data
)

os.makedirs(output_name, exist_ok=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe prefer pathlib over os

@felixhekhorn
Copy link
Contributor

This PR is mostly to keep track of @toonhasenack work, we might no merge this in the end.

Maybe this is not a priority, but I think we do want to merge because this strategy will persists also in the mid-term

@toonhasenack
Copy link
Collaborator Author

See latest push for adjustments

@alecandido
Copy link
Member

Maybe this is not a priority, but I think we do want to merge because this strategy will persists also in the mid-term

Are you sure that this should belong to Pineko? It is quite DIS specific... (even more, just specific to some observable)

@felixhekhorn
Copy link
Contributor

Maybe this is not a priority, but I think we do want to merge because this strategy will persists also in the mid-term

Are you sure that this should belong to Pineko? It is quite DIS specific... (even more, just specific to some observable)

I think it is fine to have it here, pineko has the k-factor option - of course the other option is to push it back to yadism

@felixhekhorn
Copy link
Contributor

felixhekhorn commented Mar 13, 2024

@toonhasenack please remember the comments from above and also try to unify the scripts as much as you can, they are all similar to each other

PS: as always: please also fix pre-commit

@alecandido
Copy link
Member

I think it is fine to have it here, pineko has the k-factor option - of course the other option is to push it back to yadism

Pineko is applying k-factors, but not computing them. We always kept Pineko process agnostic, and I believe that's a good thing (since all the complexity of the processes is delegated to the respective generators, and we avoid coupling to a specific one - in particular DIS, as in APFEL).

@felixhekhorn
Copy link
Contributor

I think it is fine to have it here, pineko has the k-factor option - of course the other option is to push it back to yadism

Pineko is applying k-factors, but not computing them. We always kept Pineko process agnostic, and I believe that's a good thing (since all the complexity of the processes is delegated to the respective generators, and we avoid coupling to a specific one - in particular DIS, as in APFEL).

okay, I agree - so @toonhasenack

  • please move the scripts to yadism/extras
  • keep the fixes for SV and add (at least) timelike (cc @t7phy ) and please do a proper iteration - actually, why don't we transfer all keys @andreab1997 ?

@giacomomagni
Copy link
Contributor

  • keep the fixes for SV and add (at least) timelike (cc @t7phy ) and please do a proper iteration - actually, why don't we transfer all keys @andreab1997 ?

Yes indeed that would be the proper fix. I can take care of it, here.

@andreab1997
Copy link
Contributor

I think it is fine to have it here, pineko has the k-factor option - of course the other option is to push it back to yadism

Pineko is applying k-factors, but not computing them. We always kept Pineko process agnostic, and I believe that's a good thing (since all the complexity of the processes is delegated to the respective generators, and we avoid coupling to a specific one - in particular DIS, as in APFEL).

okay, I agree - so @toonhasenack

  • please move the scripts to yadism/extras
  • keep the fixes for SV and add (at least) timelike (cc @t7phy ) and please do a proper iteration - actually, why don't we transfer all keys @andreab1997 ?

What do you mean with all keys?

@felixhekhorn
Copy link
Contributor

What do you mean with all keys?

all metadata present in original grid like result, generator info, etc. (pinefarm can add an arbitrary amount of metadata, which is also unpredictable) - I think @giacomomagni understood ... in practice only a for k,v in ori.key_values() is needed

@felixhekhorn
Copy link
Contributor

What do you mean with all keys?

all metadata present in original grid like result, generator info, etc. (pinefarm can add an arbitrary amount of metadata, which is also unpredictable) - I think @giacomomagni understood ... in practice only a for k,v in ori.key_values() is needed

this is done in #161 - so please just close @toonhasenack when you have moved the scripts

@felixhekhorn
Copy link
Contributor

@toonhasenack please don't push here. Close this one an reopen in yadism.

Comment on lines +34 to +39
for i, bin in enumerate(bins):
prediction[i] = (
bin["y"]["mid"]
* (2 - bin["y"]["mid"])
/ (bin["y"]["mid"] ** 2 + 2 * (1 - bin["y"]["mid"]))
)
Copy link
Member

Choose a reason for hiding this comment

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

@toonhasenack Here we are missing the $1/(2xF_1)$ factor.

@Radonirinaunimi Radonirinaunimi changed the title Generate kfactors for F1 division Generate kfactors for polarized observables Mar 21, 2024
@felixhekhorn
Copy link
Contributor

moved to NNPDF/yadism#270

@felixhekhorn felixhekhorn deleted the generate_kfactors branch March 22, 2024 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants