Skip to content

Fix scale variations expanded#191

Merged
andreab1997 merged 19 commits into
developfrom
feature/fix_sv
Jan 23, 2023
Merged

Fix scale variations expanded#191
andreab1997 merged 19 commits into
developfrom
feature/fix_sv

Conversation

@andreab1997
Copy link
Copy Markdown
Collaborator

@andreab1997 andreab1997 commented Jan 9, 2023

Here I just want to do two things:

@felixhekhorn @alecandido @giacomomagni

@andreab1997
Copy link
Copy Markdown
Collaborator Author

andreab1997 commented Jan 9, 2023

@giacomomagni I believe you worked on the test that is currently failing in the other PR. Can you remind me what your conclusions were? (In order to fix here)

@giacomomagni
Copy link
Copy Markdown
Collaborator

The

@giacomomagni I believe you worked on the test that is currently failing in the other PR. Can you remind me what your conclusions were? (In order to fix here)

the idea of the test was to quantify the difference between expanded and exponential (B vs A) in the case of liarized and truncated solution. So to fix it we should find where the minus sign come from. If I recall correctly your analysis shows it was due to a shift k -> -k , right?

@andreab1997
Copy link
Copy Markdown
Collaborator Author

andreab1997 commented Jan 9, 2023

The

@giacomomagni I believe you worked on the test that is currently failing in the other PR. Can you remind me what your conclusions were? (In order to fix here)

the idea of the test was to quantify the difference between expanded and exponential (B vs A) in the case of liarized and truncated solution. So to fix it we should find where the minus sign come from. If I recall correctly your analysis shows it was due to a shift k -> -k , right?

Yes indeed. At a certain point I had a doubt about the relative sign between the term gamma_0*gamma_0 and gamma_0*beta_0 but now I am convinced that is correct as it is (if we keep expanding everything in terms of alpha_s(xi_F*Q)). So yes, the shift should just be k -> -k

@felixhekhorn
Copy link
Copy Markdown
Collaborator

If I recall correctly your analysis shows it was due to a shift k -> -k , right?

ehm, if you mean by k what is called L here:

return -gamma[1] * L + 1.0 / 2.0 * (beta0 * gamma[0] + g0e2) * L**2

no - this is only true at LO SV = NLO AD, but not in the case of the snippet above

@felixhekhorn felixhekhorn added bug Something isn't working refactor Refactor code labels Jan 9, 2023
@andreab1997
Copy link
Copy Markdown
Collaborator Author

If I recall correctly your analysis shows it was due to a shift k -> -k , right?

ehm, if you mean by k what is called L here:

return -gamma[1] * L + 1.0 / 2.0 * (beta0 * gamma[0] + g0e2) * L**2

no - this is only true at LO SV = NLO AD, but not in the case of the snippet above

Sorry, my fault. With K I mean the sv kernel not the log. I should have been clearer.

@andreab1997 andreab1997 mentioned this pull request Jan 9, 2023
Comment thread src/eko/scale_variations/expanded.py Outdated
@alecandido
Copy link
Copy Markdown
Collaborator

@andreab1997 since we finally sorted out the discussion about $\alpha_s$, can you add a summary to the docs, and (if relevant) redirect to Pineko docs for a deeper discussion (where the whole picture of scale variations and their implementation, ingredients and schemes, will be described).

@andreab1997
Copy link
Copy Markdown
Collaborator Author

@felixhekhorn @alecandido I believe that (thanks also to @giacomomagni) we finally really solved the minus sign problem. As you can see the test is still failing but it would pass just changing the sign of the term beta_0 gamma_0. Indeed this is expected because we are comparing with scheme A that is defined as expanded in alpha_s(Q) while all our expressions for scheme B are expanded in alpha_s(xi_F ). Of course the actual value of alpha is not important but the two expansions should be consinstent. As you know from previous discussion, changing the alpha in the expansion exactly accounts for changing that sign that at the moment is missing. So everything is consinstent now, also with the expressions that I derived.

Of course we do not want to change the minus sign in the implementation because we agreed that we want to expand everything in terms of alpha_s(xi_F ) (so the current expression is correct), but rather we want to account for the change in alpha in the test. I will do it soon, also for N3LO.

andreab1997 and others added 2 commits January 9, 2023 17:57
Co-authored-by: Alessandro Candido <candido.ale@gmail.com>
@andreab1997
Copy link
Copy Markdown
Collaborator Author

Now the test should be ok. It was enough to remove the term that we know is generated by the change of alpha from the diff to make it pass (I left a comment but if you want I can remove it). I would say that this is the final proof that the implementation (at least up to NNLO) is correct. Let me know if you agree with this. I will soon work also to check N3LO (now that everything is clear should be simpler). I will also update the docs before merging this. @giacomomagni @felixhekhorn @alecandido

@giacomomagni
Copy link
Copy Markdown
Collaborator

giacomomagni commented Jan 11, 2023

Now the test should be ok. It was enough to remove the term that we know is generated by the change of alpha from the diff to make it pass (I left a comment but if you want I can remove it). I would say that this is the final proof that the implementation (at least up to NNLO) is correct. Let me know if you agree with this. I will soon work also to check N3LO (now that everything is clear should be simpler). I will also update the docs before merging this. @giacomomagni @felixhekhorn @alecandido

If the logic is the same at N3LO you would need to subtract: - a1**3 * b0**2 * g[0] * k**3, from the full difference A-B, right?

@andreab1997
Copy link
Copy Markdown
Collaborator Author

If the logic is the same at N3LO you would need to subtract: - a1**3 * b0**2 * g[0] * k**3, from the full difference A-B, right?

Not exactly, because at N3LO you have a lot other terms appearing. In particular, all the alpha^2 terms get a contribution at N3LO (while at NNLO only the alpha terms get a contribution from the change of alpha, which is only one term).

@giacomomagni
Copy link
Copy Markdown
Collaborator

giacomomagni commented Jan 12, 2023

To remember what has to be done:

  • fix names and conventions as here
  • Fix test of differences A-B or similar we have derived in Mathematica, at least at NLO.
    EDIT: we have fixed the tests, but we agreed it doesn't check the real difference A-B, but only the difference between kernels, not taking into account the path shifts.
    The real difference A-B has been checked to be always higher order analytically but too complicated to be tested numerically.

@andreab1997
Copy link
Copy Markdown
Collaborator Author

Can we merge this now?

Comment thread src/eko/evolution_operator/__init__.py Outdated
Comment on lines +48 to +50
Note this is NOT the real difference between scheme expanded
and exponentiated since here we don't take into account the
shifts in path length and :math:`\alpha_s` values.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why don't we also shift the alpha values, as we should? we know how to do it (it's written in the notebook)

Copy link
Copy Markdown
Collaborator

@giacomomagni giacomomagni Jan 19, 2023

Choose a reason for hiding this comment

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

This is just laziness. As mentioned above the test will not test the real HO difference A-B, since we have verified that is too complex.
This fix was derived assuming that the sv are now correct (as implemented) and checking that the difference between kernels without shifting paths/a_s is what we expect.
If you don't like it I'd propose to scratch it.

@@ -64,20 +63,50 @@ def scheme_diff(g, k, pto, is_singlet):
the accuracy of singlet quantities is slightly worse.
"""
if pto[0] >= 2:
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

do we really want a >= here? the difference strictly depends on the order - also I don't believe we want the diff +=, but instead diff =

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

As the test is build, this is correct with +=.

g02 = g[0] @ g[0] if is_singlet else g[0] ** 2
diff += a0**2 * g[1] * k - k**2 * (
1 / 2 * a0**2 * b0 * g[0] + a1 * a0 * g02 - 1 / 2 * a0**2 * g02
diff += (
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

this is the += that I meant above

Comment thread tests/eko/scale_variations/test_expanded.py
@andreab1997
Copy link
Copy Markdown
Collaborator Author

In any case I would merge this. We already checked the implemented expression for scale_varaitions with the notebook. Writing a sensible test that uses the results we derived can take a lot of time so I would open an issue for that but I would not block this PR for the test (since it is useful for several projects now). Do you agree? @giacomomagni @felixhekhorn @alecandido

@andreab1997
Copy link
Copy Markdown
Collaborator Author

Note that the issue about this test has been opened (#201 )

Copy link
Copy Markdown
Collaborator

@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 just have one comment about documentation, for the rest it looks ok.

However, I believe docs to be crucial here, since the problem is a sophisticated one, mainly about physics, more than code.

You don't have to document all in EKO, if the documentation is Pineko just add a reference to that location. But someone inspecting EKO should be able (at least through redirection) where the choices made stem from

Comment thread src/eko/scale_variations/expanded.py Outdated
@alecandido
Copy link
Copy Markdown
Collaborator

In any case I would merge this.

@andreab1997 I only had a tiny comment about documentation, and for the rest is fine. But you should reply to @felixhekhorn comments before merging.

@andreab1997
Copy link
Copy Markdown
Collaborator Author

In any case I would merge this.

@andreab1997 I only had a tiny comment about documentation, and for the rest is fine. But you should reply to @felixhekhorn comments before merging.

Ok thank you! About @felixhekhorn comments I believe @giacomomagni answered but, in any case, they are all about the test and we agreed (I believe) to open an issue for that (I already did) because for sure the test as it is is not optimal and we will need to change.

@alecandido
Copy link
Copy Markdown
Collaborator

Ok thank you! About @felixhekhorn comments I believe @giacomomagni answered but, in any case, they are all about the test and we agreed (I believe) to open an issue for that (I already did) because for sure the test as it is is not optimal and we will need to change.

If they are addressed (or delayed on purpose), people participating or who is taking the decision should resolve them. If it is just a matter of clicking a button it takes no time, but it is an explicit action to say we already took a decision.

@andreab1997
Copy link
Copy Markdown
Collaborator Author

Yes I was waiting for @felixhekhorn answer before closing the conversations :)

Copy link
Copy Markdown
Collaborator

@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.

As long as we do the correct test (maybe we can rename the current version - since it is not doing what is promised) at some point (the sooner the better), we can even merge to move forward

@andreab1997
Copy link
Copy Markdown
Collaborator Author

As long as we do the correct test (maybe we can rename the current version - since it is not doing what is promised) at some point (the sooner the better), we can even merge to move forward

The name of the test is a bit misleading but the comment explains the problem. Anyway we will rework this test in order to make it really test A vs B, so I would keep the name. If you agree, Can I merge this?

@alecandido
Copy link
Copy Markdown
Collaborator

alecandido commented Jan 23, 2023

If you'd just move the comment in 007ba72 in the docstring it would be perfect :)

@andreab1997 andreab1997 merged commit f3c4f8f into develop Jan 23, 2023
@andreab1997 andreab1997 deleted the feature/fix_sv branch January 23, 2023 17:16
@alecandido alecandido linked an issue Jan 24, 2023 that may be closed by this pull request
@felixhekhorn felixhekhorn mentioned this pull request Feb 23, 2023
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working refactor Refactor code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove alpha_raw from the game since it is not needed and wrong

4 participants