Skip to content

Commit

Permalink
Correct fencepost error in diagind. Fix #7181
Browse files Browse the repository at this point in the history
  • Loading branch information
@jiahao
jiahao committed Jun 17, 2014
1 parent 4cfb9a1 commit a16c40d
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion base/linalg/dense.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ end
function diagind(m::Integer, n::Integer, k::Integer=0)
if 0 < k < n
return range(k*m+1, m+1, min(m, n-k))
elseif 0 <= -k <= m
elseif 0 <= -k < m
return range(1-k, m+1, min(m+k,n))
end
throw(BoundsError())
Expand Down

15 comments on commit a16c40d

@JeffBezanson
Copy link
Member

Choose a reason for hiding this comment

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

Test case?

@jiahao
Copy link
Member Author

@jiahao jiahao commented on a16c40d Jun 17, 2014

Choose a reason for hiding this comment

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

What, make -C test linalg runs too quickly for you? ;)

@ViralBShah
Copy link
Member

Choose a reason for hiding this comment

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

Even with the current testsuite, we are really far from where we should be, and it is already slow. Perhaps we have reached a point where it is worth having a comprehensive testsuite and something that runs much faster.

@JeffBezanson
Copy link
Member

Choose a reason for hiding this comment

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

5-10 minutes is not a long time by test suite standards.

I used to think there should be separate fast and slow test suites, but Stefan convinced me otherwise. What do you do, run the fast suite if you feel like breaking the system, and the slow suite if you'd rather not break the system?

Actually @test itself might be slowing things down. Generally @assert is much faster, since it doesn't go through try/catch and the test handler machinery. We should see if it's worth having a mode that converts tests to asserts, which still checks everything, but with less detailed reporting.

@jiahao
Copy link
Member Author

@jiahao jiahao commented on a16c40d Jun 17, 2014

Choose a reason for hiding this comment

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

Test added in 2c02e0b

@ViralBShah
Copy link
Member

Choose a reason for hiding this comment

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

A system of complexity such as julia really needs to have a comprehensive testsuite. It made sense so far not to do so, but I am not sure I agree going forward. Anyways, this is not the place for that discussion, and it is not as if anyone (including me) is volunteering to write a whole bunch of tests.

@JeffBezanson
Copy link
Member

Choose a reason for hiding this comment

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

I'm pretty sure everybody agrees we want the most comprehensive testsuite possible. If we were deliberately trying to make the testsuite not comprehensive, I wasn't aware of it.

@lindahua
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should at least have a coverage report as to how much of the codebase has been covered by the test suites.

@andreasnoack
Copy link
Member

Choose a reason for hiding this comment

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

This one broke some code of mine because the code used that you got

julia> diag(zeros(0,0))
0-element Array{Float64,1}

Would it be a problem if either diagind returned an empty range instead of an exception for abs(k)>=n or just that it did for abs(k)==n?

@jiahao
Copy link
Member Author

@jiahao jiahao commented on a16c40d Jun 30, 2014

Choose a reason for hiding this comment

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

I think an empty range would be acceptable in all cases where it now throws a BoundsError()

@StefanKarpinski
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's ok for all cases where we throw a bounds error, just the corner cases where an empty range is correct.

@StefanKarpinski
Copy link
Member

Choose a reason for hiding this comment

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

I'm working on a patch for this but the tests are giving me a bit of trouble.

@StefanKarpinski
Copy link
Member

Choose a reason for hiding this comment

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

And that would be because I'm editing the code in one copy of Julia and the tests in another.

@StefanKarpinski
Copy link
Member

Choose a reason for hiding this comment

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

7dd97fa

So the actual fencepost error in the original code was that it should have been 0 < k <= n instead of 0 < k < n. My change basically does that but I decided to tersen up the code a bit and make it IMO clearer and add some more tests, of course.

@andreasnoack
Copy link
Member

Choose a reason for hiding this comment

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

Great, thanks.

Please sign in to comment.