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

Implement the light part of the polarized coefficient functions #173

Merged
merged 76 commits into from May 17, 2023

Conversation

Radonirinaunimi
Copy link
Member

@Radonirinaunimi Radonirinaunimi commented Feb 22, 2023

Implement the massless spin-dependent coefficient functions $g_1$, $g_4$ and $g_L$ (up to NNLO).

Status of the NC implementation:

SF LO NLO [NS, Gluon] NNLO [NS, Gluon, S]
g1 [✅, ✅] [✅, ✅ , ✅]
g4 [✅, 0] [✅, 0 , 0]
gL 0 [✅, 0] [✅, 0 , 0]

TODO:

  • Apfel++ benchmark and parity violating couplings fixes.
  • Target Mass Correction (implemented for $g_1$)
  • CC implementation (should be straight forward) same relations ad NC (Omitted)
  • TMC for polarized observables (delegated to Polarized FONLL coefficient functions #187)

Radonirinaunimi and others added 3 commits February 25, 2023 23:32
Co-authored-by: Felix Hekhorn <felixhekhorn@users.noreply.github.com>
Co-authored-by: Tanjona Rabemananjara <rrabeman@nikhef.nl>
@Radonirinaunimi
Copy link
Member Author

Also, while wanting to document the polarized TMC (given that the approximations were not available, for example), I noticed that there were materials in extra/TMC that are out of date/no longer needed. I assume that these should be cleaned up?

@alecandido
Copy link
Member

Also, while wanting to document the polarized TMC (given that the approximations were not available, for example), I noticed that there were materials in extra/TMC that are out of date/no longer needed. I assume that these should be cleaned up?

extras/ is mostly preserving the historic memory of yadism development, to reconstruct how and why we did things.
For sure at some point we might want to clean up things, but for the time being we are busy enough with higher priorities, so personally I will not invest much energy in it soon.

@Radonirinaunimi
Copy link
Member Author

extras/ is mostly preserving the historic memory of yadism development, to reconstruct how and why we did things. For sure at some point we might want to clean up things, but for the time being we are busy enough with higher priorities, so personally I will not invest much energy in it soon.

I understand, it was more of since I will be touching that folder I might as well do so. Just wanted to know if you'd agree that this should be taken care of in the future.

@alecandido
Copy link
Member

I understand, it was more of since I will be touching that folder I might as well do so. Just wanted to know if you'd agree that this should be taken care of in the future.

In the future for sure (something we'll want to keep, but definitely there are stuffs to be dropped).

For the time being, my advice is to just open a new folder inside, name after your topic, and put your stuffs in. Just forget about the rest 😅

Copy link
Contributor

@felixhekhorn felixhekhorn left a comment

Choose a reason for hiding this comment

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

Convert comments on 9943e65#comments to review (sorry)

src/yadism/esf/tmc.py Show resolved Hide resolved
src/yadism/esf/tmc.py Outdated Show resolved Hide resolved
src/yadism/esf/tmc.py Outdated Show resolved Hide resolved
src/yadism/esf/tmc.py Show resolved Hide resolved
src/yadism/esf/tmc.py Outdated Show resolved Hide resolved
@Radonirinaunimi
Copy link
Member Author

Are you happy with these now @felixhekhorn?

@felixhekhorn
Copy link
Contributor

apart from the potential $F_3$ bug this should be fine ...

@giacomomagni
Copy link
Collaborator

apart from the potential F3 bug this should be fine ...

Do you mean g1 == F3 for the NS ?

@Radonirinaunimi
Copy link
Member Author

apart from the potential F3 bug this should be fine ...

Do you mean g1 == F3 for the NS ?

No, I believe Felix meant this comment:

https://github.com/NNPDF/yadism/pull/173/files#r1183519543

@Radonirinaunimi
Copy link
Member Author

Radonirinaunimi commented May 16, 2023

@felixhekhorn, I agree that the h3_ker should be equal to k1_ker and I have fixed it in da50b10.

With this, this PR could now be merged.

@alecandido
Copy link
Member

Part of the breaking tests are breaking because we do not support py3.7 any longer, at least in EKO (and consequently also in yadism, at least because of the dependency).

It should be sufficient to upgrade the version range of regression to be the same of unit tests.

However, even py3.8-9 are failing, and here the workflow complains about a missing requirements.txt file.
Should we disregard these tests at all (then deactivating), solve here, or solve in a further PR (i.e. open now an issue)?

Copy link
Member

@alecandido alecandido left a comment

Choose a reason for hiding this comment

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

I haven't reviewed yet the NNLO numbers (and I won't do it, of course) and TMC.

However, what I saw is perfectly fine, so you could also consider merging :)

benchmarks/runners/apfel_.py Show resolved Hide resolved
@@ -50,6 +50,13 @@ def name(self):
"""joint name"""
return self.kind + "_" + self.flavor

@property
def is_parity_violating(self):
Copy link
Member

Choose a reason for hiding this comment

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

Not a strong opinion, but for these names I usually prefer dropping the is_ for simplicity.
On one side it is more or less the same, on the other it is slightly more consistent, because it is a property (the is_something() is more consistent for a function, since it is an action, but a property is supposed to be a pseudo-attribute, so it should pretend to be an entity, not an action).

Copy link
Member Author

Choose a reason for hiding this comment

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

This, I do not also have a strong opinion, but at least doing it this way is consistent with what are currently there:

@property
def is_heavy(self):

@property
def is_raw_heavy(self):

@property
def is_asy(self):

@property
def is_heavylight(self):

@property
def is_composed(self):

So, in order to be consistent everywhere we'd also need to replace the rests, which probably should better be done in a different PR.

src/yadism/coefficient_functions/light/kernels.py Outdated Show resolved Hide resolved
src/yadmark/benchmark/external/apfel_utils.py Show resolved Hide resolved
@Radonirinaunimi
Copy link
Member Author

Part of the breaking tests are breaking because we do not support py3.7 any longer, at least in EKO (and consequently also in yadism, at least because of the dependency).

It should be sufficient to upgrade the version range of regression to be the same of unit tests.

However, even py3.8-9 are failing, and here the workflow complains about a missing requirements.txt file. Should we disregard these tests at all (then deactivating), solve here, or solve in a further PR (i.e. open now an issue)?

I was indeed wondering about this, but honestly I think that doing it in another PR would be the best course of actions, if not for the git history.

@alecandido
Copy link
Member

I was indeed wondering about this, but honestly I think that doing it in another PR would be the best course of actions, if not for the git history.

The main reason why I pointed it out, it's exactly that those workflows are not running in master, so they are being reintroduced by this PR.

@felixhekhorn
Copy link
Contributor

I was indeed wondering about this, but honestly I think that doing it in another PR would be the best course of actions, if not for the git history.

The main reason why I pointed it out, it's exactly that those workflows are not running in master, so they are being reintroduced by this PR.

Well I deactivated them (as said in #190 ) in master because I realised here they were failing and they were stupid - just merge master here (dropping the workflow)

@Radonirinaunimi
Copy link
Member Author

@alecandido Are you happy for this to be merged now?

Copy link
Member

@alecandido alecandido left a comment

Choose a reason for hiding this comment

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

About TMC, I have the feeling there might be too much copy-pasting around, but in general (it has been simply passed a lot of time since I took a look at it).

I would not block this PR certainly because of that, and other refactors are better coupled to numerical FONLL implementation, rather than polarized.

@Radonirinaunimi
Copy link
Member Author

About TMC, I have the feeling there might be too much copy-pasting around, but in general (it has been simply passed a lot of time since I took a look at it).

I would not block this PR certainly because of that, and other refactors are better coupled to numerical FONLL implementation, rather than polarized.

Ok, I'm merging this now.

As for the TMC, apart from the bug in $F_3$ that has already been there, there isn't that much. There is only the _k functions implemented in the base class, which currently could be moved outside as they are only used by $g_1$, but when $g_2$ will be implemented in the future these functions will have to be in the base class.

@Radonirinaunimi Radonirinaunimi merged commit b22431c into master May 17, 2023
3 of 4 checks passed
@Radonirinaunimi Radonirinaunimi deleted the polarized-coeffs branch May 17, 2023 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request physics physics features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants