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

Error requiring LoopVectorization from Tullio #51

Closed
jondeuce opened this issue Dec 1, 2020 · 7 comments · Fixed by #53
Closed

Error requiring LoopVectorization from Tullio #51

jondeuce opened this issue Dec 1, 2020 · 7 comments · Fixed by #53

Comments

@jondeuce
Copy link

jondeuce commented Dec 1, 2020

I get the following warning on Tullio v0.2.10 when loading LoopVectorization v0.9.1:

┌ Warning: Error requiring LoopVectorization from Tullio:
│ UndefVarError: SVec not defined
...

I believe it is due to the renaming of an internal struct SVec in previous versions of VectorizationBase to Vec in current versions.

I'm not familiar enough with your internals to make a PR to fix this, but naively it looks like a simple find and replace.

@mcabbott
Copy link
Owner

mcabbott commented Dec 1, 2020

On master there's a quick attempt at a fix, much like you suggest: 7896059

The awkward bit about using Requires is that we can't let Pkg bound versions. But maybe this is simple enough to just work. Needs to be tested a bit more...

@jondeuce
Copy link
Author

jondeuce commented Dec 1, 2020

Thanks for the quick response! Ahh yes, I see the problem, that's a bit of a sticky situation. Your fix appears to have worked for the original issue, but now I receive a similar warning when loading ForwardDiff:

┌ Warning: Error requiring ForwardDiff from Tullio:
│ LoadError: UndefVarError: SVec not defined
│ ...in expression starting at /home/.julia/packages/Tullio/JZOUh/src/grad/avxdual.jl:6

It seems like another small change should fix this, too; the culprit should be this line.

@mcabbott
Copy link
Owner

mcabbott commented Dec 1, 2020

Thanks for testing... mostly I should comment out the worked example in that file! With latest commit:

julia> using Tullio, ForwardDiff, LoopVectorization, Tracker

julia> r33 = rand(3,3);

julia> Tracker.gradient(x -> sum(@tullio y[i] := log(x[i,j])), r33)
([1.1394564210213642 2.1680893862937927 1.0559383908137436; 1.1351873479437349 6.481911983174566 14.998379183695935; 1.85023371959207 19.48339920489614 1.0349811616209528] (tracked),)

julia> Tracker.gradient(x -> sum(@tullio y[i] := log(x[i,j]) grad=Dual), r33)
([1.1394564210213642 2.1680893862937927 1.0559383908137436; 1.1351873479437349 6.481911983174566 14.998379183695935; 1.85023371959207 19.48339920489614 1.0349811616209528] (tracked),)

@chriselrod
Copy link
Contributor

You're not the first person to have issues from the renaming. It'd be nice if optional dependencies could specify bounds.

Maybe I should have defined:

const SVec = Vec

@jondeuce
Copy link
Author

jondeuce commented Dec 2, 2020

Great - seems like everything is working on my end 👍.

@chriselrod I believe that would have been sufficient in this case, though cluttering up your package's namespace is not ideal. But as you say, specifying bounds on optional dependencies (and generally being able to test different versions of optional dependencies more easily) would be much better.

@AshtonSBradley
Copy link

so... what is the fix?

@mcabbott
Copy link
Owner

mcabbott commented Dec 3, 2020

@AshtonSBradley the solution is to use master for now, as in ] add Tullio#master. There's a fix but I wanted to test it a bit more carefully before tagging.

@chriselrod no problem at all, this is a pretty minimal level of hassle. Thanks for sorting out the 1.6 update.

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 a pull request may close this issue.

4 participants