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

hep: Fixes bug in gamma_trace of sum of products containing GammaMatrix mixed with other symbols #23786

Merged
merged 4 commits into from
Jul 29, 2022

Conversation

glenn-horton-smith
Copy link
Contributor

@glenn-horton-smith glenn-horton-smith commented Jul 16, 2022

References to other Issues or PRs

Fixes #13636

Brief description of what is fixed or changed

… of GammaMatrix mixed with other factors.
@sympy-bot
Copy link

sympy-bot commented Jul 16, 2022

Hi, I am the SymPy bot (v167). I'm here to help you write a release notes entry. Please read the guide on how to write release notes.

Your release notes are in good order.

Here is what the release notes will look like:

This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.12.

Click here to see the pull request description that was parsed.
#### References to other Issues or PRs

Fixes #13636 

#### Brief description of what is fixed or changed

<!-- BEGIN RELEASE NOTES -->
* physics.hep
  * Fixed a bug in finding traces of sums of products of GammaMatrix mixed with other factors. (#13636)
<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

@github-actions
Copy link

Benchmark results from GitHub Actions

Lower numbers are good, higher numbers are bad. A ratio less than 1
means a speed up and greater than 1 means a slowdown. Green lines
beginning with + are slowdowns (the PR is slower then master or
master is slower than the previous release). Red lines beginning
with - are speedups.

Significantly changed benchmark results (PR vs master)

Significantly changed benchmark results (master vs previous release)

Full benchmark results can be found as artifacts in GitHub Actions
(click on checks at the top of the PR).

@glenn-horton-smith
Copy link
Contributor Author

I don't know what to do about failing "test / optional-dependencies (3.11.0-alpha - 3.11, true) (pull_request)". It's completely unrelated to the change I made, and seems to be happening to everyone on all pull requests now.

@oscarbenjamin
Copy link
Collaborator

Yes, that's #23774.

It's fine if that test fails. It is not "required" to pass.

@@ -588,6 +588,8 @@ Gilbert Gede <[email protected]> <[email protected]>
Gilles Schintgen <[email protected]>
Gina <[email protected]>
Gleb Siroki <[email protected]>
Glenn Horton-Smith <[email protected]>
Glenn Horton-Smith <[email protected]> Glenn Horton-Smith <[email protected]>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think only the first of these lines is needed. It doesn't look like the ksu.edu email is used in any of the commits. Did you add that because of an error reported by the mailmap script?

If you actually wanted your entry in the AUTHORS file to be recorded under ksu.edu then it should be the other way around i.e.:

Glenn Horton-Smith <[email protected]> Glenn Horton-Smith <[email protected]>

That way it is saying that the gmail address is an alias for the ksu address which should be the one recorded in the AUTHORS file and associated with these commits.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not needed for any current commit. I can remove the second line if you like. I only did it because I have "email = [email protected]" as a global setting on my work computer and I thought it might be useful in case I ever made a commit from work without remembering to set the email correctly.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The question is just which email address you would like to be recorded in AUTHORS. If you want it to be the gmail address then the above is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, ok. Yes, I want it to be the gmail address. Thanks for asking.

@@ -399,3 +400,25 @@ def test_gamma_matrix_trace():
t = ps*ps*ps*ps*ps*ps*ps*ps
r = gamma_trace(t)
assert r.equals(4*p2*p2*p2*p2)

def test_bug_13636():
Copy link
Collaborator

Choose a reason for hiding this comment

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

Usually there should be two blank lines between top-level functions, classes etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is fixed now.

Comment on lines 419 to 424
assert ta.equals(
-16 * ki(i0) * ki(-i0) * pf(i1) * pi(-i1)
+ 32 * ki(i0) * ki(i1) * pf(-i0) * pi(-i1)
)
assert tb.equals(-8 * x * ki(i0) * pf(-i0) * pi(i1) * pi(-i1))
assert t_a_plus_b.equals(ta + tb)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does == not work here or does it need to use .equals?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. I just checked, and indeed == does not work here. The .equals() function for tensor objects applies sympify and .cannon_bp to the tensor expression before comparing, while .eq is just Basic.eq, and is false for this comparison because the expression trees are not exactly identical. The other test functions such as test_gamma_matrix_trace() also use .equals().

Copy link
Collaborator

Choose a reason for hiding this comment

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

Usually it is better to construct the exact expression for comparison in tests so that they can be compared with ==. Sometimes that is not be best option because the exact expression is confusing and does not display the intent of the test well.

I'm not sure what the preferred answer would be here. What exactly is the difference between the compared expressions? You can use srepr as one way to view the expression structure explicitly.

Copy link
Contributor Author

@glenn-horton-smith glenn-horton-smith Jul 28, 2022

Choose a reason for hiding this comment

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

ta == 4*((-4)*ki(L_0)*ki(-L_0)*pf(L_1)*pi(-L_1) + 8*ki(L_0)*ki(L_1)*pf(-L_0)*pi(-L_1)) which is the same as the expression being tested against except for distribution of the factor of 4 over the coefficients. I can use the exact expression for the test, I don't mind.

tb and ta_plus_tb are the same as the expressions they are being compared against, so we could use == there with no problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is fixed now.

@oscarbenjamin
Copy link
Collaborator

I can't personally verify the correctness of the code or the test from a mathematical perspective. Maybe @jksuom, @sami-b95 or @certik can?

If not then I'm happy to merge this but I'll wait to see if anyone can check it.

This particular module is not maintained which makes it difficult to merge PRs. As far as I know none of the people who actively merge PRs has expert knowledge of either the module or the subject matter. @glenn-horton-smith if you are interested in maintaining/improving this then that would be very welcome.

@certik
Copy link
Member

certik commented Jul 24, 2022

I think this is correct. All it is doing is a test to make sure the sum of traces is the same as trace of the sum.

@oscarbenjamin
Copy link
Collaborator

Thanks @certik. Could you also check #23823?

I'll wait for @glenn-horton-smith to respond to my other comments but otherwise this is good to merge.

@certik
Copy link
Member

certik commented Jul 24, 2022

Thanks @certik. Could you also check #23823?

Done.

@oscarbenjamin
Copy link
Collaborator

Thanks @certik!

Also some small whitespace changes for readability and style.
@oscarbenjamin
Copy link
Collaborator

The pypy failure and the 3.11 failure are both unrelated to the changes here.

@glenn-horton-smith
Copy link
Contributor Author

Thanks @certik and @oscarbenjamin ! Have I missed responding to anything?

@oscarbenjamin
Copy link
Collaborator

Looks good to me.

@oscarbenjamin oscarbenjamin merged commit c9e427b into sympy:master Jul 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Strangely behaved gamma_trace (gamma matrix trace)
4 participants