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

[NDTensors] Add StridedViews dependency and fix using statement #1238

Merged
merged 3 commits into from
Nov 6, 2023

Conversation

emstoudenmire
Copy link
Collaborator

Description

Before this change, NDTensors was failing to precompile. This was after making sure I had updated various dependencies and taken both NDTensors and ITensors out of 'dev' mode as checks that it wasn't for any of these reasons.

It seems that StridedViews.jl is now its own separate package, so no longer inside of the latest version of Strided.jl (see this comment in the What's New section of Strided.jl's README). This PR adds StridedViews as a dependency and does using StridedViews directly instead of the previous code doing using Strided.StridedViews.

@mtfishman
Copy link
Member

That's strange, I don't understand why the current code would have been failing. For example this works:

julia> using Strided.StridedViews

julia> StridedView(@view randn(4, 4)'[1:2, 1:2])
2×2 StridedView{Float64, 2, Matrix{Float64}, typeof(identity)}:
 -0.501281    -0.0232372
  0.00611145   1.82511

i.e. it's fine to load a dependency of a package in that way, even if it is a separate package.

I'm not against this change since it's reasonable to make use of StridedViews directly but I have a feeling your precompilation issue is caused by something else.

@codecov-commenter
Copy link

codecov-commenter commented Nov 4, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (9a7380f) 85.35% compared to head (bbd1bed) 54.56%.
Report is 1 commits behind head on main.

❗ Current head bbd1bed differs from pull request most recent head 6a0b653. Consider uploading reports for the commit 6a0b653 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    #1238       +/-   ##
===========================================
- Coverage   85.35%   54.56%   -30.79%     
===========================================
  Files          89       88        -1     
  Lines        8445     8392       -53     
===========================================
- Hits         7208     4579     -2629     
- Misses       1237     3813     +2576     

see 37 files with indirect coverage changes

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

@mtfishman mtfishman changed the title Add StridedViews dependency and fix using statement [NDTensors] Add StridedViews dependency and fix using statement Nov 4, 2023
@emstoudenmire
Copy link
Collaborator Author

Helpful to know. I was interpreting the notation Strided.StridedViews differently in my head, as if StridedViews had to be somehow a module inside of Strided i.e. not a separate package (which it now is). But since that notation can work even if it refers to a separate package, it must have been an issue on my side.

Sure enough, my Strided.jl version was out of date.

If you prefer this change, please go ahead and merge or I can but if not we can close this PR.

@mtfishman
Copy link
Member

Could you add a compat entry of 0.2 into the Project.toml for StridedViews? Also we should drop support for older versions of Strided and just support 2, allowing older versions of Strided is probably why that issue was allowed to happen on your end, we should only depend on more recent versions of Strided which had StridedViews separated out.

@emstoudenmire
Copy link
Collaborator Author

emstoudenmire commented Nov 4, 2023

Great. I modified the following lines of NDTensors Project.toml:

Strided = "2"
StridedViews = "0.2"

Please let me know if you think it'd be better to put, say, 2.0.4 for Strided or any other change like that.

(The line about Requires was just something the Julia package manager did when I added StridedViews through the REPL.)

@mtfishman mtfishman merged commit 2dd057b into main Nov 6, 2023
7 checks passed
@mtfishman mtfishman deleted the stridedviews branch November 6, 2023 19:27
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