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

[NDTensorsMetalExt] Fix more scalar indexing issues on Metal #1264

Merged
merged 80 commits into from
Dec 1, 2023

Conversation

kmp5VT
Copy link
Collaborator

@kmp5VT kmp5VT commented Nov 16, 2023

Description

Trying to get Metal up to speed as CUDA is and have it be testable in the NDTensors/test suite.

Fixes #(issue)

Remove scalar indexing but also running into issues with Float64 typing in NDTensors.

  • Changing the file names in NDTensors/test to test_filename.jl and writing the loop to go over test_*.jl
  • In each test file I am going to add a @safetestset to the beginning of the file and put the using statements in the safetestset section.
  • I'll explicitly write which functions are included from each of the modules in all of the test files.
  • In files where metal breaks because of eltype issues I will migrate Tensor(...) constructors to Tensor{elt}(...) and loop over eltypes.

@codecov-commenter
Copy link

codecov-commenter commented Nov 16, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d7167b8) 84.06% compared to head (f6d3e50) 53.94%.

❗ Current head f6d3e50 differs from pull request most recent head a2d569d. Consider uploading reports for the commit a2d569d to get more accurate results

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #1264       +/-   ##
===========================================
- Coverage   84.06%   53.94%   -30.13%     
===========================================
  Files          99       98        -1     
  Lines        8542     8489       -53     
===========================================
- Hits         7181     4579     -2602     
- Misses       1361     3910     +2549     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kmp5VT kmp5VT marked this pull request as draft November 17, 2023 16:49
@mtfishman mtfishman changed the title Same issue on Metal and CUDA [NDTensorsMetalExt] Fix more scalar indexing issues on Metal Nov 17, 2023
@kmp5VT
Copy link
Collaborator Author

kmp5VT commented Nov 20, 2023

@mtfishman To recap our conversation and as a refresher for me. The notes for changes are as follows

  1. Changing the file names in NDTensors/test to test_filename.jl and writing the loop to go over test_*.jl
  2. In each test file I am going to add a @safetestset to the beginning of the file and put the using statements in the safetestset section.
  3. I'll explicitly write which functions are included from each of the modules in all of the test files.
  4. In files where metal breaks because of eltype issues I will migrate Tensor(...) constructors to Tensor{elt}(...) and loop over eltypes.

@kmp5VT kmp5VT marked this pull request as ready for review November 30, 2023 20:03
@kmp5VT
Copy link
Collaborator Author

kmp5VT commented Nov 30, 2023

@mtfishman I believe this is ready for review

@kmp5VT
Copy link
Collaborator Author

kmp5VT commented Dec 1, 2023

@mtfishman I am not sure why these tests are failing. When I test with Julia 1.9 all pass but it looks like theres an issue with sort in julia 1.6 and 1.8. I didn't make any modifications to the code that is failing. To verify I grabbed the last passing version of this branch (tag b56f8a4) and ran ]test NDTensors in julia 1.8 and it also fails in the sort function in blocksparse contraction with the combiner. Do you have any idea what is happening?

@mtfishman
Copy link
Member

May be related to JuliaLang/Compat.jl#812 and JuliaLang/julia#52010, sorting tuples was temporarily supporting in Base Julia but then removed.

In places where sort(::Tuple) is failing we can try to change the call to TupleTools.sort(...).

@mtfishman
Copy link
Member

Thanks for the quick fix to the tuple sorting issue. I'll merge this so we can register a new version.

@mtfishman mtfishman merged commit 1aa5f39 into ITensor:main Dec 1, 2023
8 checks passed
@kmp5VT kmp5VT deleted the kmp5/debug/scalar_metal branch December 3, 2023 04:00
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.

3 participants