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

Remove unnecessary method specializations for Numbers #2014

Merged
merged 1 commit into from
Sep 27, 2024

Conversation

dennisYatunin
Copy link
Member

@dennisYatunin dennisYatunin commented Sep 27, 2024

There appear to be inference issues arising from unnecessary specializations on Number types in several RecursiveApply methods. Specifically, the specializations for rdiv (but not for rmul) are the source of the first bug described in #1983, and removing them fixes that error. Removing these specializations may also fix other inference problems for operations on complex fields (e.g., matrix fields of non-scalar values).

Several specializations of rmuladd are not removed in this PR because rmap does not currently support 3 input arguments (only 1 or 2). This can be fixed in the future, especially if we end up rewriting rmap in terms of unrolled_map.

  • Code follows the style guidelines OR N/A.
  • Unit tests are included OR N/A.
  • Code is exercised in an integration test OR N/A.
  • Documentation has been added/updated OR N/A.

Copy link
Member

@charleskawczynski charleskawczynski left a comment

Choose a reason for hiding this comment

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

This is my favorite kind of PR, better yet favorite way to fix inference failures: delete code. Fantastic.

@charleskawczynski
Copy link
Member

Actually, @dennisYatunin, would you mind adding a JET test for the case that is breaking on the main branch?

@dennisYatunin dennisYatunin force-pushed the dy/number_methods branch 2 times, most recently from 55fecb8 to c26dab8 Compare September 27, 2024 21:16
@dennisYatunin
Copy link
Member Author

I just added a new test_non_scalar_5 unit test, which divides a matrix field of Covariant12Vectors by a scalar. On my machine, this test fails with the Number specializations and passes without them. If the CPU and GPU tests on CI pass, I think this PR is good to merge in. Unfortunately, it doesn't appear to fix any of the soft fails we currently have in our CI, but it definitely fixes the first part of #1983.

@charleskawczynski
Copy link
Member

Excellent, thanks!

@dennisYatunin
Copy link
Member Author

Wow, looks like the CPU test on CI allocates 16 bytes for the in-place broadcast expression, even though the version I ran locally doesn't allocate anything. The only difference I can see is that I'm running Julia 1.10.4, while CI is running Julia 1.10.5. @charleskawczynski, I think this might be indicating another compiler regression?

Although the CPU test allocates, both it and the GPU test are able to run without crashing. So, this PR is definitely improving inference, and I'll merge it in with the CPU test as a soft-fail for now. We can look into the allocations issue in a separate PR.

@charleskawczynski charleskawczynski merged commit 846edd8 into main Sep 27, 2024
21 of 22 checks passed
@charleskawczynski charleskawczynski deleted the dy/number_methods branch September 27, 2024 23:15
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.

2 participants