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

WIP: convert to stagedfunctions #26

Closed
wants to merge 6 commits into from
Closed

WIP: convert to stagedfunctions #26

wants to merge 6 commits into from

Conversation

timholy
Copy link
Member

@timholy timholy commented Feb 17, 2015

This is a WIP to fix #21. It mostly works, and has a massively-beneficial impact on package loading time, but I am getting a truly bizarre error. I inserted some printing code to illustrate the problem:

julia> include("runtests.jl")
Testing Linear interpolation in 1D...
Testing Linear interpolation in 2D...
Testing Quadratic interpolation in 1D...
Testing Quadratic interpolation in 2D...
Testing evaluation on grid and boundary in 1D...
Testing evaluation on grid and boundary in 2D...
Testing evaluation on grid and boundary in 3D...
Testing gradient evaluation
g = [-0.0]
g = [-0.0]
g = [-0.0]
g = [-0.0]
g = [-0.0]
g = [-0.0]
g = [-0.0]
g = [-0.0]
g = [0.0]
g = [0.0]
g = [0.0]
g = [0.0]
g = [0.0]
g = [0.0]
g = [0.0]
g = [0.0]
g = [-0.0]
g = [-0.0]
g = [-0.0]
g = [-0.0]
g = [0.15043281484715354]
gradient!(g,itp1,x) = [-0.6782138028608058,-0.99190379965505,-0.8414709848078965,-0.2973045442608362,0.3859739967178289,0.8886510150090671,0.9755183471216421,0.6059298029372212,-0.04718003020117026]
g = [0.15043281484715354]
g = [0.15043281484715354]
ERROR: LoadError: LoadError: assertion failed: |g1(x) - (gradient!(g,itp1,x))[1]| <= 0.015353177135101162
  g1(x) = 0.15353177135101162
  (gradient!(g,itp1,x))[1] = 1
  difference = 0.8464682286489884 > 0.015353177135101162
 in error at error.jl:20
 in test_approx_eq at test.jl:137
 in anonymous at no file:27
 in include at ./boot.jl:249
 in include_from_node1 at ./loading.jl:128
 in include at ./boot.jl:249
 in include_from_node1 at ./loading.jl:128
while loading /home/tim/.julia/v0.4/Interpolations/test/gradient.jl, in expression starting on line 24
while loading /home/tim/.julia/v0.4/Interpolations/test/runtests.jl, in expression starting on line 16

Upon further investigation, despite the code gradient! is returning itp1.coefs rather than g, but only from the direct gradient! call and not from the gradient wrapper (which just calls gradient!). Also note that there's an inconsistency in (gradient!(g,itp1,x))[1] = 1 with gradient!(g,itp1,x) = [-0.6782138028608058,-0.99190379965505,-0.8414709848078965,-0.2973045442608362,0.3859739967178289,0.8886510150090671,0.9755183471216421,0.6059298029372212,-0.04718003020117026].

I got to the point where I can't work on this any more tonight, and I'm pretty baffled. Any ideas? I suspect this is a bug in julia proper.

Loading time is much shorter now!

This also updates the REQUIRE file to include julia 0.4 and the
new WoodburyMatrices package (since Woodbury was deleted from base)
@tomasaschan
Copy link
Contributor

Thanks a lot for starting this off!

The error is indeed bizarre. I have no idea what could cause this, especially since - as you already demonstrated - it doesn't even seem to be consistent between display and indexing into the gradient vectors. Could it be a problem with re-using g for some reason? What happens if you pass Array(Float64,nx) instead? (If that is a problem, we need to fix it anyway, but it's nice to have things to rule out...)

I have a few nitpicks and questions about the code too, but I'll submit them as line comments shortly.

ret
end
end
supported(::Type{Constant{OnGrid}}) = true
Copy link
Contributor

Choose a reason for hiding this comment

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

Is supported used anywhere by us, or is it just a type trait that can be used elsewhere?

Copy link
Member Author

Choose a reason for hiding this comment

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

No uses yet. I was keying off the # This creates getindex methods for all supported combinations and wondered if you wanted to be able to return an error like "not a supported combination" from something. So since the list was handy, I turned it into a trait. If we don't end up using it at all, then these should be deleted.

@timholy
Copy link
Member Author

timholy commented Feb 17, 2015

Thanks for the speedy review!

There didn't seem to be a magic bullet for the weird error. I'll keep plugging away at it.

This seems to have been triggered by mixing string
interpolation and expression interpolation.
@timholy
Copy link
Member Author

timholy commented Feb 17, 2015

Got it. It was a bizarre consequence of mixing string interpolation and expression interpolation. For the purposes of base julia, I'll see if I can come up with a reduced test case, but meanwhile here's a version that passes all tests for me locally.

Package loading time is now 0.7 seconds on my laptop. On a different machine still running an older version of 0.4 (from before the great GC merge), package load time was 38.5 seconds. A nice improvement, I think!

@timholy
Copy link
Member Author

timholy commented Feb 17, 2015

Before merging I'm happy to squash. I just left them as separate commits so you could see what changed.

This package is now usable only on julia 0.4 or higher
@timholy
Copy link
Member Author

timholy commented Feb 17, 2015

Now we are green. Turns out I needed to register the WoodburyMatrices package.

@tomasaschan
Copy link
Contributor

Nice find! I've never been nitpicky with git history, so feel free to squash or not whichever you prefer. When I start my Ubuntu machine next I'll tag a version of Interpolations.jl that has the latest changes in a 0.3-compatible state, and then I'll merge this if you're happy with it (and tag again, for all the 0.4 users out there).

Again, thanks a lot for tackling this rewrite for me! This really does bring the package a huge step toward 1.0.

@timholy
Copy link
Member Author

timholy commented Feb 18, 2015

Merged locally (with a rebase) in 6da1207. This way links will persist in case I want to refer to this conversation for seeing if this weird stagedfunction bug is a genuine julia bug.

@timholy timholy closed this Feb 18, 2015
@tomasaschan
Copy link
Contributor

@timholy I was going to use these improvements to take a new stab at #24, but I can't figure out what to check out to get these changes locally. Did you commit stuff to master locally without pushing?

@tomasaschan
Copy link
Contributor

nvm, figured it out. git fetch must have failed for some reason, because the local branch heads weren't updated.

@timholy timholy deleted the teh/staged branch August 7, 2015 22:53
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.

Startup times & stagedfunctions
2 participants