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

Caching harmonic sums #202

Closed
wants to merge 32 commits into from
Closed

Caching harmonic sums #202

wants to merge 32 commits into from

Conversation

felixhekhorn
Copy link
Contributor

@felixhekhorn felixhekhorn commented Jan 26, 2023

Here is my proposal for a central harmonic sums cache. Main concepts:

  • we drag one cache array around which is not worse than dragging the sx array around (and better than the two in Cache harmonics sums for the as2 anomalous dimensions #179 ); furthermore this would solve Removing deprecated list of list in harmonic cache #143 , since we resort to a plain list
  • we fill this array slowly whenever we request a new harmonic sum
  • and so the moment we compute a new harmonic sum, we register it in the cache
  • the central task for everybody that needs a harmonic sum is to register a "key" (in cache.py) (Note that the order does not matter, since we would never use the number of the key, but only its name)
  • when computing the actual harmonic sum we even could go recursive and have e.g. $g_3$ and $S_{-2,1}$ simultaneously around (we can either go recursive or just do a straight return there, á là if key == Sm21: return get(g3)+n+zeta, or we inline the relation where the thing is needed)

Please tell me what you think!

The single commit here is just to lay out the idea at LO and by no means complete ...

cc @enocera

TODO

  • move harmonic constants to global constants

@giacomomagni
Copy link
Collaborator

giacomomagni commented Jan 27, 2023

Hi @felixhekhorn thanks for this idea, looks good to me! I have two minor comments:

  • We should add another argument is_singlet or eta needed for more complex harmonics
  • We should define a naming convention for S(1,N/2), S(1, (N-1)/2) ... and similar

Co-authored-by: Giacomo Magni <39065935+giacomomagni@users.noreply.github.com>
@alecandido
Copy link
Member

alecandido commented Jan 27, 2023

@felixhekhorn are you sure that Numba has no issue with globals?

EDIT: in particular https://numba.pydata.org/numba-doc/dev/user/faq.html#numba-doesn-t-seem-to-care-when-i-modify-a-global-variable

@felixhekhorn
Copy link
Contributor Author

S1 = $S_1(N)$
S1h = $S(1,N/2)$
S1mh = $S(1, (N-1)/2)$
Sm1 = $S(-1,N)$

Base automatically changed from split-math-in-module to master January 30, 2023 16:25
…and added harmonic constants to eko.constants module
@t7phy
Copy link
Member

t7phy commented Jan 30, 2023

Hi,

  • I have added all the harmonic sums to the cache that are currently implemented in the w files. I have however not yet added the harmonic sums with N/2 and (N-1)/2 as I was not sure up to which order they need to be added. (let me know which ones are needed and I could update them)
  • I have also added the harmonic constants to eko.constants module and updated the corresponding imports.
  • I also added the bool parameter is_singlet to the cache function to allow consistency with the Smx functions.

Perhaps have a look and if its alright, we could merge this also and use the new cache in the splitting functions.

@@ -1,7 +1,12 @@
Features
========

<<<<<<< HEAD
Copy link
Contributor Author

Choose a reason for hiding this comment

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

now you have some nice merge conflicts - that you need to resolve ;-) (and might it be that you're still not running pre-commit? because that should have prevented you from committing thing ...)

Copy link
Member

Choose a reason for hiding this comment

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

I have been using pre commit for the time_like branch but not here because pydocs requires imperative statements in docstrings(for instance, it needs to be Compute xyz instead of Computes xyz), however a lot of the previous functions (such as in as2, as3, etc.) do not implement it so I just skip pre commit for this branch for the time being. Will get to it soon enough ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know - but for new code you should, of course, do it and then you are only allowed to skip pre-commit in the very last step, i.e. when no one, but pydocstyle is complaining

Copy link
Member

Choose a reason for hiding this comment

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

right, noted!

@t7phy
Copy link
Member

t7phy commented Feb 10, 2023

@felixhekhorn @niclaurenti @giacomomagni @alecandido I have modified the complete space-like implementation to be compatible with the new harmonic cache (ADs (ASx and AEMx) and OMEs). Please have a look.

In principal it should be fine to merge in of itself, however as all the tests were based on old cache system, they are failing. I have updated many of them but there is just too many for me to update on my own.

-------
ome : numpy.ndarray
matching operator matrix
<<<<<<< HEAD:src/eko/evolution_operator/operator_matrix_element.py
Copy link
Collaborator

@giacomomagni giacomomagni Feb 10, 2023

Choose a reason for hiding this comment

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

here there is an unsolved conflict. The change coming from master is the correct one (same below)



@nb.njit(cache=True)
def gamma_nsm(n, nf, sx):
def gamma_nsm(n, nf, cache, is_singlet):
Copy link
Collaborator

@giacomomagni giacomomagni Feb 10, 2023

Choose a reason for hiding this comment

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

I think is_singlet should not appear as argument in any function except for nsp which can be used as singlet and as non singlet.
Calling this function with is_singlet=True is misleading, so we should not have this possibility.
Viceversa hold for gg, gq, qg, ps ...
@niclaurenti is the expert for the mixed QED QCD one.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @giacomomagni . In this case you don't need to pass is_singlet because you already know that we are not in the singlet sector. It is better to call get with is_singlet=False (however notice that is_singlet is useless for the first harmonic sums so consider using a default argument like is_singlet=None)

@alecandido
Copy link
Member

What is actually missing from this PR? I.e. why is this still draft?

Please, put checkboxes in the OP for all the items, and tick those that you believe to be completed, even if they can be improved (everything can always be improved).
In general, we should try to merge smaller PR, to reduce the amount of changes and improve the review process.

@giacomomagni
Copy link
Collaborator

giacomomagni commented Apr 28, 2023

Given that #148 and QED have been merged, it's a good time to take care of this.
@t7phy if it's okay for you I can work a bit on this ?

@t7phy
Copy link
Member

t7phy commented Apr 28, 2023

Given that #148 has been merged, it's a good time to take care of this.
@t7phy if it's okay for you I can work a bit on this ?

Yes, please go ahead.

@giacomomagni
Copy link
Collaborator

okay there are endless confilcts in basically all files... maybe it's easier to start from scratch?

@niclaurenti
Copy link
Contributor

Given that #148 and QED have been merged, it's a good time to take care of this.
@t7phy if it's okay for you I can work a bit on this ?

If you need help with the harmonic sums needed in the QED sector I can help

@giacomomagni
Copy link
Collaborator

giacomomagni commented Apr 28, 2023

If you need help with the harmonic sums needed in the QED sector I can help

I was trying to merge master here again, but everything (almost all the files in ekore display conflicts), we have changed
Time like, QED and N3LO in the meantime + all the refactors...
I'd say we can start again in a new branch. The two main ideas were:

  1. We move all the constants in to eko.constants.
  2. We introduce the harmonic cache as done here.

Was there anything else to save from here @felixhekhorn @t7phy ?

@t7phy
Copy link
Member

t7phy commented Apr 28, 2023

If you need help with the harmonic sums needed in the QED sector I can help

I was trying to merge master here again, but everything (almost all the files in ekore display conflicts), we have changed
Time like, QED and N3LO in the meantime + all the refactors...
I'd say we can start again in a new branch. The two main ideas were:

  1. We move all the constants in to eko.constants.
  2. We introduce the harmonic cache as done here.

Was there anything else to save from here @felixhekhorn @t7phy ?

Yes I agree, I think starting in new branch is simpler. The two files are the only main changes to copy. Ofcourse, you could also copy all the space like ADs and OMEs as I had already added the new harmonic sums to them.

@felixhekhorn
Copy link
Contributor Author

I was trying to merge master here again, but everything (almost all the files in ekore display conflicts), we have changed Time like, QED and N3LO in the meantime + all the refactors... I'd say we can start again in a new branch. The two main ideas were:

1. We move all the constants in to `eko.constants`.

2. We introduce the harmonic cache as done here.

Was there anything else to save from here @felixhekhorn @t7phy ?

I agree, a clean restart might be better ... (solving the is_singlet along the way)

@giacomomagni
Copy link
Collaborator

closed in favor of #252 and new PR

@felixhekhorn felixhekhorn mentioned this pull request Apr 28, 2023
4 tasks
@giacomomagni giacomomagni deleted the harmonic-sums-cache branch May 3, 2023 11:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Refactor code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Removing deprecated list of list in harmonic cache
6 participants