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

Time-like Anomalous dimensions #200

Closed
wants to merge 22 commits into from
Closed

Time-like Anomalous dimensions #200

wants to merge 22 commits into from

Conversation

t7phy
Copy link
Member

@t7phy t7phy commented Jan 20, 2023

Added the LO Anomalous dimensions as starters, hence the PR is currently set to draft

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.

Thanks @t7phy for this PR!

In general, it looks good to me, I just left some comments to improve the style, by making it uniform to the rest of the code.
Concerning EKO itself, you can look at the other modules to have a reference (I guess you already done):
https://github.com/NNPDF/eko/blob/time_like_unpolarized/src/ekore/anomalous_dimensions/unpolarized/space_like/as1.py
while the official reference we follow for the docstrings is:
https://www.sphinx-doc.org/en/master/usage/extensions/example_numpy.html

Having uniform docstrings is not top-priority, but we try to stick to a single convention in order to minimize future changes (and possibly to make it easier to automate them).

@alecandido
Copy link
Member

@t7phy just another tiny advice: commits comments are useful to get a quick summary of changes done, and to rapidly identify which are the responsible comments, in case you're looking for something specific.

Writing great commit messages is terribly difficult and annoying, so lots of people are doing this poorly, and everyone is sloppy sometimes.
But if you can, try to be as descriptive as you can, keeping it brief (a short sentence for each commit is definitely enough).
image

@@ -1,21 +0,0 @@
r"""The unpolarized, time-like Altarelli-Parisi splitting kernels.
Copy link
Contributor

Choose a reason for hiding this comment

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

this file you need for sure since it is the entry point to your implementation - so instead of deleting it you should adjust it (take a look to the space-like counterpart)

Copy link
Member Author

Choose a reason for hiding this comment

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

yes indeed, I had only removed it so I could add the actual functions once I am done with the as2 and as3 modules.

Copy link
Contributor

Choose a reason for hiding this comment

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

but you don't need to delete the file because of that, no? and for sure we start with LO first and once we established that we step forward ...

Copy link
Member Author

Choose a reason for hiding this comment

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

the file is still there, I had removed the functions which stated it is not implemented, to make way for the actual functions implementing the splitting functions. I will add the functions asap.

LO quark-quark anomalous dimension $\gamma_{qq}^{(0)}(N)$

"""
return constants.CF * (-3.0 + (4.0 * s1) - 2.0 / (N * (N + 1.0)))
Copy link
Contributor

Choose a reason for hiding this comment

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

this is the same as

so you should not reimplement, but just redirect (same of course for all the others); only gamma_singlet changes if I understood correctly (so that one you keep)

Copy link
Member Author

@t7phy t7phy Jan 23, 2023

Choose a reason for hiding this comment

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

well actually, only the 2 functions, namely gamma_ns and gamma_gg remain the same. gamma_gq and gamma_qg do differ by factors of nf. I think it would be better and clearer if we have the _ns and _gg functions clearly specified and not redirect to the space-like folder because it provides a cleaner and clearer separation between TL and SL and also allows for proper citation of references with functions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Even fine ... are there relations beyond LO between TL and SL?

Copy link
Member Author

Choose a reason for hiding this comment

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

no, its only for the LO afaik, for NLO and NNLO, they vary, so there will be different functions there anyway

Copy link
Contributor

Choose a reason for hiding this comment

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

Then I'm also fine with duplicating two lines ... just remember to add a unit test that actually checks that

@felixhekhorn felixhekhorn added enhancement New feature or request physics new physics features labels Jan 23, 2023
Base automatically changed from split-math-in-module to master January 30, 2023 16:25
@felixhekhorn
Copy link
Contributor

felixhekhorn commented Feb 1, 2023

  • If you want to build on top of Caching harmonic sums #202 consider to rebase this PR else you can not test your code
  • in any case I would suggest you strongly to first do the LO benchmark before going up in order

return result

@nb.njit(cache=True)
def gamma_gg(N, nf, cache, is_singlet=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def gamma_gg(N, nf, cache, is_singlet=None):
def gamma_gg(N, nf, cache):

you know you are in the singlet case here - so no need for the argument (this is of course also true for all other functions)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I know that, however in #202, I have modified the cache.get to take is_singlet as an argument as well. Since that function is called in gamma_qq and gamma_gg for the s1, I just set the is_singlet to None to ensure no missing arguments are there.

Indeed I will have to rebase this branch, and that's why I have been trying to complete the #202 to have it fully working.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I know that, however in #202, I have modified the cache.get to take is_singlet as an argument as well. Since that function is called in gamma_qq and gamma_gg for the s1, I just set the is_singlet to None to ensure no missing arguments are there.

mh? how you mean? yes, you need to pass the argument to get, but this does not mean it has to be an argument for gg - on the contrary, gg enforces the argument to be True (forever) - or am I missing something?

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed I will have to rebase this branch, and that's why I have been trying to complete the #202 to have it fully working.

I don't expect #202 to be closed any time soon ... so you should keep #202 working (as it was) and gradually increase the code basis whenever you make a new step

Copy link
Member Author

Choose a reason for hiding this comment

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

mh? how you mean? yes, you need to pass the argument to get, but this does not mean it has to be an argument for gg - on the contrary, gg enforces the argument to be True (forever) - or am I missing something?

It does not enforce to to be True. It sets it to be None, and then when the gamma_singlet is defined, it is then set to True, as it should be. I did this as a way to keep the code consistent across all orders. A little more than needed, I agree, but cleaner imho.

Copy link
Contributor

Choose a reason for hiding this comment

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

I still don't see this point (and I forgot to ask during the meeting) - @giacomomagni may I ask you for a comment?

Copy link
Member Author

Choose a reason for hiding this comment

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

well, its just a matter of code consistency across perturbative orders. At LO, in principal, I could hard code the cache.get to have is_singlet = True but, this way, the code is consistent such that every splitting function that uses cache, needs an argument is_singlet, and this is specified in the init file making it very consistent.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry I'm a bit confused... I need to check #202. But I believe a function called gamma_gg can only be singlet,
so the argument should not be need otherwise it's really confusing. Instead is_singlet should be enforced in all the harmonics called inside this function.

@felixhekhorn
Copy link
Contributor

Also please remember to install pre-commit ... (the longer you delay the more mess it will create 🙃 )

@t7phy
Copy link
Member Author

t7phy commented Feb 1, 2023

Also please remember to install pre-commit ... (the longer you delay the more mess it will create upside_down_face )

I did install it, but there seems to be some installation issues with a few of the hooks, and I couldn't find a solution to fix them quickly. I will try to figure it out.

@felixhekhorn felixhekhorn mentioned this pull request Feb 8, 2023
1 task
@@ -0,0 +1,21 @@
r"""The polarized, time-like Altarelli-Parisi splitting kernels.
Copy link
Contributor

Choose a reason for hiding this comment

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

this file is deleted in master (likewise for OME)

:math:`\gamma_{qq}^{(0)}(N)`

"""
s1 = w1.S1(N) # c.get(c.S1, cache, N, is_singlet)
Copy link
Contributor

Choose a reason for hiding this comment

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

If you wish you can immediately step into #202 , i.e. base this branch on top of the other

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 have eliminated the conflicts from this branch for the moment, perhaps I should eliminate them from #202 as well before this step

Copy link
Collaborator

@giacomomagni giacomomagni left a comment

Choose a reason for hiding this comment

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

Hi @t7phy I left some minor comments, mainly about doc strings.
Looks quite good!

I suggest you, to make a decision for the harmonics, otherwise it's confusing:

  • either you stick to the old notation and you aim to close this before Caching harmonic sums #202.
  • either you base this on Caching harmonic sums #202, having in mind that that has to be merged before closing this. (PS I'll try to give an hand on the other one, if I have time :) )


See Also
--------
ekore.anomalous_dimensions.unpolarized.time_like.as1.gamma_ns : :math:`\gamma_{ns}^{(0)}(N)`
Copy link
Collaborator

@giacomomagni giacomomagni Feb 9, 2023

Choose a reason for hiding this comment

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

I know we have this see also section in the corresponding unpolarised space like, but I would remove it everywhere
and not repeat it. It was added at the beginning of the project, I believe. I find it not useful anymore...

Copy link
Member

Choose a reason for hiding this comment

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

Agreed

@@ -0,0 +1,175 @@
"""The unpolarized LO Altarelli-Parisi splitting kernels."""
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
"""The unpolarized LO Altarelli-Parisi splitting kernels."""
"""The unpolarized |LO| Altarelli-Parisi splitting kernels."""

same for |NLO| and |NNLO| in all the docstring.

return result

@nb.njit(cache=True)
def gamma_gg(N, nf, cache, is_singlet=None):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry I'm a bit confused... I need to check #202. But I believe a function called gamma_gg can only be singlet,
so the argument should not be need otherwise it's really confusing. Instead is_singlet should be enforced in all the harmonics called inside this function.

m1tpN = 1
elif is_singlet == False:
m1tpN = -1
else:
Copy link
Collaborator

@giacomomagni giacomomagni Feb 9, 2023

Choose a reason for hiding this comment

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

I think this case with npp(-1,N) should not appear. A quantity is either continued with m1tpN=1 or m1tpN=-1.



@nb.njit(cache=True)
def gamma_qqs(N, nf):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know this is not consistent everywhere, but we usually call this gamma_ps or gamma_qq_ps.
@felixhekhorn we should make these name more uniform.

Copy link
Contributor

Choose a reason for hiding this comment

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

agreed

@nb.njit(cache=True)
def gamma_nsp(N, nf, cache, is_singlet=None):
r"""Compute the NNLO non-singlet positive anomalous dimension.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Remember to add the reference to the source code / publication in all these functions.

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.

Mainly tiny comments, but here benchmarks are very reduced (maybe only APFEL available? Consider adding at least APFEL++ then) and no tests provided.

I got this is not yet depending on #202, so, if possible, I would push to test and document a working LO, and only push for NLO and NNLO in further PRs.
It's not very useful to implement a large bunch of code without any intermediate test, and if we keep for long as a branch we (i.e. the branch author) will pay over and over the price of merging/rebasing on master.

@@ -106,6 +106,8 @@ def __init__(
if order == (1, 0) and method != "iterate-exact":
logger.warning("Evolution: In LO we use the exact solution always!")

logger.info(("Time" if configs.time_like else "Space") + "-like evolution")
Copy link
Member

Choose a reason for hiding this comment

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

As I proposed for polarized, let's make this machine-friendly:

Suggested change
logger.info(("Time" if configs.time_like else "Space") + "-like evolution")
logger.info(dict(evol_kind="time" if configs.time_like else "space"))

or, definitely simpler, not sure if better:

Suggested change
logger.info(("Time" if configs.time_like else "Space") + "-like evolution")
logger.info(dict(time_like=configs.time_like))

@felixhekhorn ?

Copy link
Contributor

Choose a reason for hiding this comment

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

let's follow #195 - so the latter

@@ -36,7 +36,6 @@ def build_ome(A, matching_order, a_s, backward_method):
-------
ome : numpy.ndarray
matching operator matrix

Copy link
Member

Choose a reason for hiding this comment

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

Keep these lines, they are part of our docstrings format.


See Also
--------
ekore.anomalous_dimensions.unpolarized.time_like.as1.gamma_ns : :math:`\gamma_{ns}^{(0)}(N)`
Copy link
Member

Choose a reason for hiding this comment

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

Agreed

def gamma_qq(N, cache, is_singlet=None):
r"""Compute the LO quark-quark anomalous dimension.

Implements Eqn. (B.3) from :cite:`Mitov:2006wy`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Implements Eqn. (B.3) from :cite:`Mitov:2006wy`.
Implements :eqref:`B.3` from :cite:`Mitov:2006wy`.

see:

.. role:: eqref
:class: eqref

:cite:`Liu:2015fxa` :eqref:`5`.

@@ -2,9 +2,10 @@

import ekore.anomalous_dimensions.unpolarized.time_like as ad_ut

# tests to be added later
Copy link
Member

Choose a reason for hiding this comment

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

Better now than later

import numpy as np

# from eko.constants import zeta2
from numpy import power as npp
Copy link
Member

Choose a reason for hiding this comment

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

Python has a built-in power operator, i.e. **. Numba supports it, as much as numpy.power, so you will get the same code.

Please, favor built-ins over third-party extension functions (including NumPy).



@nb.njit(cache=True)
def gamma_nsp(N, nf, cache=None, is_singlet=None):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def gamma_nsp(N, nf, cache=None, is_singlet=None):
def gamma_nsp(N, nf, cache=None, is_singlet=False):

is_singlet has to be a boolean, you're also declaring as such in the docstring. Refrain from assign a value of a different type.

s2 = w2.S2(N) # c.get(c.S2, cache, N, is_singlet)
sm2 = w2.Sm2(N, s2, True) # c.get(c.Sm2, cache, N, is_singlet)

m1tpN = 1
Copy link
Member

Choose a reason for hiding this comment

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

Since it's 1, you can also drop it. I hope it will be optimized away, but at least it would uselessly increase compile-time.

Comment on lines +613 to +623
+ 1152 * zeta2 * npp(N, 2)
+ 1248 * zeta2 * npp(N, 3)
- 1296 * zeta2 * npp(N, 4)
- 792 * zeta2 * npp(N, 5)
+ 876 * zeta2 * npp(N, 6)
- 1368 * zeta2 * npp(N, 7)
- 2340 * zeta2 * npp(N, 8)
+ 120 * zeta2 * npp(N, 9)
+ 1476 * zeta2 * npp(N, 10)
+ 792 * zeta2 * npp(N, 11)
+ 132 * zeta2 * npp(N, 12)
Copy link
Member

Choose a reason for hiding this comment

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

Collect things to make them readable and more efficient. Mathematica can help you to do this, if you need.



@nb.njit(cache=True)
def gamma_singlet(N, nf, cache, is_singlet=True):
Copy link
Member

Choose a reason for hiding this comment

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

Why do you need an is_singlet argument? It is written in the name of the function that is a singlet.

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 is an old, not used branch, I have left it still just for reference. This PR should also be closed. The branch tlu_ad_v2 is the place with actual implementation that is ready.

Comment on lines +815 to +816
is_singlet : boolean
True for singlet, False for non-singlet, None otherwise
Copy link
Member

Choose a reason for hiding this comment

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

If it were actually three-valued, you should have written:

Suggested change
is_singlet : boolean
True for singlet, False for non-singlet, None otherwise
is_singlet : boolean or None
True for singlet, False for non-singlet, None otherwise

But, as @giacomomagni wrote above, None makes no sense, and, as I wrote above, here even False makes no sense: it is a singlet quantity.

@alecandido
Copy link
Member

alecandido commented Mar 25, 2023

@t7phy, re #200 (comment), please create a new PR for the new one:
https://github.com/NNPDF/eko/compare/tlu_ad_v2
Without a PR is difficult to track progresses for us, and to help you, looking at the code status. The PR interface provides a useful interface to discuss code and track conversations, that's why we use it a lot :)

As soon as you will have done, I'll close this (you can do it yourself, if you wish).

(This commit 7307c6e happened 10 days ago, and missing another PR I thought this was the branch.)

@t7phy t7phy closed this Mar 25, 2023
@t7phy t7phy reopened this Mar 25, 2023
@t7phy
Copy link
Member Author

t7phy commented Mar 25, 2023

closed in favor of #232

@t7phy t7phy closed this Mar 25, 2023
@giacomomagni giacomomagni deleted the time_like_unpolarized branch March 30, 2023 12:24
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 new physics features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants