Fix harmonics by giacomomagni · Pull Request #110 · NNPDF/eko

giacomomagni · 2022-03-29T15:21:22Z

Move harmonics to a sub-package
Use the cached harmonics in the NLO anomalous dimension (to remove explict mellin_g3 calls, need to rewrite all the ad expressions probably not worth... )
Use the cached harmonics in the NNLO matching (remove explict mellin_g3 calls )
More clean harmonics cache ordering inside N3LO matching
Expand docs section about harmonics

alecandido · 2022-03-29T15:34:11Z

@giacomomagni thank you very much for taking care of properly dealing with harmonics.

Actually, @niclaurenti was asking me about:

Use the cached harmonics in the NLO anomalous dimension (remove explict mellin_g3 calls )

this same afternoon. It seems like cached is not used at all in NLO QCD.

The moment you finish, please ask for a review also @niclaurenti.

codecov · 2022-03-29T15:36:34Z

Codecov Report

Merging #110 (9606618) into feature/N3LO_matching (e6ee4bf) will not change coverage.
The diff coverage is 100.00%.

@@                   Coverage Diff                   @@
##           feature/N3LO_matching      #110   +/-   ##
=======================================================
  Coverage                 100.00%   100.00%           
=======================================================
  Files                         59        65    +6     
  Lines                       2965      3060   +95     
=======================================================
+ Hits                        2965      3060   +95

Flag	Coverage Δ
isobench	`54.33% <33.19%> (-0.54%)`	⬇️
unittests	`100.00% <100.00%> (+0.06%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/eko/anomalous_dimensions/as1.py	`100.00% <ø> (ø)`
src/eko/evolution_operator/__init__.py	`100.00% <ø> (ø)`
src/eko/harmonics/f_functions/__init__.py	`100.00% <ø> (ø)`
src/eko/harmonics/f_functions/f11.py	`100.00% <ø> (ø)`
src/eko/harmonics/f_functions/f13.py	`100.00% <ø> (ø)`
src/eko/harmonics/f_functions/f14_f12.py	`100.00% <ø> (ø)`
src/eko/harmonics/f_functions/f16.py	`100.00% <ø> (ø)`
src/eko/harmonics/f_functions/f17.py	`100.00% <ø> (ø)`
src/eko/harmonics/f_functions/f18.py	`100.00% <ø> (ø)`
src/eko/harmonics/f_functions/f19.py	`100.00% <ø> (ø)`
... and 31 more

felixhekhorn · 2022-03-29T15:55:38Z

It seems like cached is not used at all in NLO QCD.

Just to confirm, this is true - moreover

https://github.com/N3PDF/eko/blob/341e6323e680d2320ae4400da61c92a445160416/src/eko/anomalous_dimensions/nlo.py#L5-L7

this is(/was) because NLO is also a special case inside PEGASUS - and I didn't wanted to duplicate that structure

alecandido · 2022-03-29T16:04:38Z

I guess at that time there was no point in worrying about a different and slower NLO, we had to write NNLO in the first place.

But now that we have eko working, do you still believe that we don't want to use cached harmonics? @felixhekhorn

It might be an optimization for the NLO runs.

felixhekhorn · 2022-03-29T16:10:04Z

do you still believe that we don't want to use cached harmonics? @felixhekhorn

It might be an optimization for the NLO runs.

definitely we want them! (Something we wanted to do since long ago ...) (I was just explaining why I decided like this back then)

felixhekhorn · 2022-03-31T13:11:12Z

If I may suggest a crazy idea - and you can tell me whether you like it or not and whether it's feasable ...

we could go even a more radical step and enforce more the caching of harmonics (which we do believe is our biggest time consumer):

we could even try to catch and cache such a line: https://github.com/N3PDF/eko/blob/d782a61ccc952e394533ccd20e2c81f49eea1f86/src/eko/harmonics/g_functions.py#L92-L93
because we actually only need to compute S1(N) once and for all - and that's the complicated part
but then S1(N+1) can be computed via the usual recurrence relations
since it will be complicated at this point we would need a class as a manager
so it would become something like h.get("S1", N, j)

alecandido · 2022-04-05T09:23:35Z

@alecandido I added asv as dev-dependency by hand in pyproject.toml (4e5c2b1), but that apparently was breaking - do I need to run poetry update in addition? (I already ran for 10min and then gave up ...)

If you don't tell poetry to update the lockfile in any way, then you will end up in an inconsistent state between your pyproject.toml and poetry.lock, thus it's fair that it complaints...

You can do one of three things:

if you want to update to the latest compatible versions all your dependencies, then yes, you have to do poetry update
if you just want to generate a new lockfile, you can just use poetry lock
if you just want to add a dependency, please consider not doing it by hand, but using poetry add (or poetry add -D for dev dependencies)

felixhekhorn · 2022-04-05T10:51:40Z

@giacomomagni in 44759d0 I wanted to add a few tests to check the d_m - but then I realized, the way I implemented should not work, because S_{-1} really is a complicated object, i.e. it depends on being singlet-like or not ...

and again: https://github.com/N3PDF/eko/blob/44759d0ba93db67d8addf796f251dc5764ed7252/src/eko/harmonics/w1.py#L62
this should never happen! (as far as I understand)

Please double check the mathematics in this branch! Actually even looking at #83 I can see some (-1)**n

giacomomagni · 2022-04-05T11:39:10Z

I thought we discussed this in the past and we agreed that the factors (-1)**N couldn't appear explicitly inside the expressions of the ad/ome, but where only inside the harmonics...

alecandido · 2022-04-06T08:56:56Z

Expand docs section about harmonics

I will review, but I would tick also the last item before merging. You were right in putting it in :)

alecandido

Benchmarks and documentation are fine (as usual: we should stop using benchmarks in sandbox mode as soon as possible...)

alecandido

Tests are mostly good (I'm just suggesting some aesthetic and readability), they even run in a reasonable amount of time (even though ome takes a bit, but that's fine).

alecandido · 2022-04-06T11:57:59Z

            # due to the approximations of F functions.
            np.testing.assert_allclose(
-                aS3[1, 1], ref_val_qqNS[L][idx] + ref_val_qqPS[L][idx], rtol=3e-2
+                aS3[1, 1], ref_val_qqNS[L][idx] + ref_val_qqPS[L][idx], rtol=8e-2


This approximation looks really bad...

So here the difference it's from the fact that we are testing even moments, but in A_qqNS we have replaced all the occurrences of (-1)^N -> -1 (explicit and implicit in the harmonics). This is not fully possible in Mathematica unless you expand all the harmonics also there...
On top of this effect there is even the approximation of the F functions, but the former should be the leading,
so I'd say this difference is expected.

Ok, I get the technical limitation. At this point I just wonder if we should drop the test.
However, we're always in time to do it.

felixhekhorn

I didn't re-review everything, but I hope it is fine - and we need this stuff soonish in develop ...

giacomomagni added 3 commits March 29, 2022 17:16

move harmonics to a proper subpackage

6eb55c7

update tests accordingly

1611c51

other minor updates

5533c3f

giacomomagni mentioned this pull request Mar 29, 2022

N3LO matching #83

Merged

first attempt for the harmoncs cache

fd01b7a

felixhekhorn reviewed Mar 30, 2022

View reviewed changes

Comment thread src/eko/matching_conditions/as2.py Outdated

felixhekhorn reviewed Mar 30, 2022

View reviewed changes

Comment thread src/eko/matching_conditions/as2.py Outdated

felixhekhorn reviewed Mar 30, 2022

View reviewed changes

Comment thread src/eko/matching_conditions/as2.py Outdated

giacomomagni added 9 commits March 30, 2022 14:05

restore old signature in ome as1 and as2, delete conftest

aeed018

Move get_s3x and get_s4x to harmonics

bcaf20b

update as3 ome harmincs and tests

174a8e8

remove old n3lo names

cd48236

passing S1 and S2 to nlo ad

18078f9

Fix sx doc in as3

fb75e69

fix docs in ope as1 and as2

40e388e

fix docs in as1

b1d5b63

fix harmonics in benchmarks

562f3c9

alecandido reviewed Mar 30, 2022

View reviewed changes

Comment thread src/eko/harmonics/__init__.py Outdated

giacomomagni added 5 commits March 31, 2022 10:12

remove get_

aafb65a

use sx in ad init

25c744a

fix benchmarks

c072ebf

remove old stuff in test ad nnlo

1eb8ef2

start reordering harmonics in ome as3

d782a61

giacomomagni commented Mar 31, 2022

View reviewed changes

Comment thread src/eko/matching_conditions/as3/__init__.py Outdated

felixhekhorn added 2 commits April 5, 2022 11:27

Call ev_op.compute once in parallel

81ad0b5

Update Mellin doc

4aa9cd9

felixhekhorn reviewed Apr 5, 2022

View reviewed changes

Comment thread doc/source/theory/Mellin.rst Outdated

felixhekhorn added 3 commits April 5, 2022 12:11

Update docstrings

2d5482f

Update lock file

bb9693c

Attempt to add more harmonic tests

44759d0

giacomomagni added 5 commits April 5, 2022 13:50

fix w5 docstring

8c9e7d3

Add is_singlet to harmonics sums and remove (-1)**N

deee6a2

Adding is_singlet to tests

f44a1f6

Allow for generic complex S minus harmonics

f76f97c

remove impossible odd moment test on ahq3

fa9f688

niclaurenti approved these changes Apr 6, 2022

View reviewed changes

passing correct harmonics to as3.A_qqNS

510c741

giacomomagni added 3 commits April 6, 2022 11:19

Add S minus test against old definition

4319726

remove useless minus sign

87065d2

expand harmonics docs

b06d813

giacomomagni requested a review from felixhekhorn April 6, 2022 09:22

alecandido reviewed Apr 6, 2022

View reviewed changes

Comment thread benchmarks/performance/harmonics.py Outdated

alecandido reviewed Apr 6, 2022

View reviewed changes

giacomomagni and others added 3 commits April 6, 2022 15:35

suggested fixes in tests

febe124

Remove commented code

c59aa0f

Unify non-singlet language

9606618

felixhekhorn approved these changes Apr 6, 2022

View reviewed changes

alecandido assigned giacomomagni Apr 8, 2022

giacomomagni merged commit 9606618 into feature/N3LO_matching Apr 12, 2022

delete-merged-branch Bot deleted the feature/harmonics branch April 12, 2022 14:08

Conversation

giacomomagni commented Mar 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alecandido commented Mar 29, 2022

Uh oh!

codecov Bot commented Mar 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

felixhekhorn commented Mar 29, 2022

Uh oh!

alecandido commented Mar 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

felixhekhorn commented Mar 29, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

felixhekhorn commented Mar 31, 2022

Uh oh!

alecandido commented Apr 5, 2022

Uh oh!

Uh oh!

felixhekhorn commented Apr 5, 2022

Uh oh!

giacomomagni commented Apr 5, 2022

Uh oh!

alecandido commented Apr 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alecandido left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alecandido left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alecandido Apr 6, 2022

Choose a reason for hiding this comment

Uh oh!

giacomomagni Apr 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alecandido Apr 6, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

felixhekhorn left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

giacomomagni commented Mar 29, 2022 •

edited

Loading

codecov Bot commented Mar 29, 2022 •

edited

Loading

alecandido commented Mar 29, 2022 •

edited

Loading

alecandido commented Apr 6, 2022 •

edited

Loading

alecandido left a comment •

edited

Loading

giacomomagni Apr 6, 2022 •

edited

Loading