Skip to content

Fixed alloc_matmul to handle more than 3 dimensions.#7

Closed
nhipsman wants to merge 2 commits intotimholy:masterfrom
nhipsman:edit
Closed

Fixed alloc_matmul to handle more than 3 dimensions.#7
nhipsman wants to merge 2 commits intotimholy:masterfrom
nhipsman:edit

Conversation

@nhipsman
Copy link
Copy Markdown
Contributor

Simple issue with the way Julia allocates arrays of higher dimension. Did this to fix #151 on JuliaMath/Interpolations.jl.

My first pull request (ever), so if I've broken any official or unofficial rules of contributing, I'd be appreciative if you let me know.

@codecov-io
Copy link
Copy Markdown

codecov-io commented Apr 24, 2017

Codecov Report

Merging #7 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master       #7   +/-   ##
=======================================
  Coverage   99.37%   99.37%           
=======================================
  Files           4        4           
  Lines         159      159           
=======================================
  Hits          158      158           
  Misses          1        1
Impacted Files Coverage Δ
src/AxisAlgorithms.jl 85.71% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6cbd6b9...530dec4. Read the comment docs.

@timholy
Copy link
Copy Markdown
Owner

timholy commented Apr 24, 2017

Thanks @nhipsman! Adding a test would be great, but for your first pull request this seems good enough to me. I'm honored!

@timholy
Copy link
Copy Markdown
Owner

timholy commented Apr 24, 2017

I was looking into writing a test before merging this, and on julia 0.5 this already seems to work. Just to make sure I'm not missing something, are you using Julia 0.4?

@nhipsman
Copy link
Copy Markdown
Contributor Author

Using 0.5.1. I get the error (that I mentioned in #151 on Interpolations.jl) running it on either Linux or macOS (haven't tried Windows yet). I have also recently updated my packages, and am using Interpolations v 0.6.0. Hmmm...

Regarding tests - I figured that would be a good idea, but I'm not sure how that's usually done - can you point me somewhere that explains the proper form for such a test?

@timholy
Copy link
Copy Markdown
Owner

timholy commented Apr 24, 2017

Check out https://github.com/timholy/AxisAlgorithms.jl/blob/master/test/tridiag.jl. What I usually do is add the test first (so I know it's catching the error) and then make the fix. In this case I guess the easiest would be to copy/paste the error you reported in JuliaMath/Interpolations.jl#151. We'd need to add a file test/REQUIRE with the contents

Interpolations

so Interpolations.jl gets installed on Travis when we run the tests.

If this is confusing, I can merge this and add the test myself, but I'll give you a chance to decide for yourself.

@timholy
Copy link
Copy Markdown
Owner

timholy commented Apr 24, 2017

Or perhaps the test/woodbury.jl file. Looks like if you just make the test matrix 4d rather than 3d, you trigger the same error. I was confused initially because I didn't realize it was specific for Woodbury matrices. EDIT: if you do that, you don't necessarily have to invoke Interpolations.

@nhipsman
Copy link
Copy Markdown
Contributor Author

I think that's what you were looking for? Thanks for the guidance!

@timholy
Copy link
Copy Markdown
Owner

timholy commented May 24, 2019

@nhipsman, I owe you a sincere apology. I missed that you had finished this off. I should have merged this ages ago. The new version makes this unnecessary, I'm afraid, so I will have to close this. I regret not merging your first PR!

@timholy
Copy link
Copy Markdown
Owner

timholy commented May 24, 2019

Actually, in #13 I decided to commit the test component of this, and credited it to you.

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