Skip to content

lapack/gonum: prevent division of untyped ints always yielding zero in Dlasy2#1653

Merged
vladimir-ch merged 2 commits intogonum:masterfrom
soypat:divz
Jun 11, 2021
Merged

lapack/gonum: prevent division of untyped ints always yielding zero in Dlasy2#1653
vladimir-ch merged 2 commits intogonum:masterfrom
soypat:divz

Conversation

@soypat
Copy link
Contributor

@soypat soypat commented Jun 5, 2021

Please take a look at the line I changed. The following operation:

scale = 1 / 8 / maxbtmp

always yields zero. I added decimal points to 1 and 8 to fix this undesired behavior.

@soypat soypat changed the title prevent division of untyped ints always yielding zero gonum/lapack/dlasy2.go: prevent division of untyped ints always yielding zero Jun 5, 2021
@soypat soypat changed the title gonum/lapack/dlasy2.go: prevent division of untyped ints always yielding zero lapack/gonum/dlasy2.go: prevent division of untyped ints always yielding zero Jun 5, 2021
Copy link
Member

@kortschak kortschak left a comment

Choose a reason for hiding this comment

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

This branch is not tested, so it would be nice if we could get an input that exercises this case.

@codecov-commenter
Copy link

codecov-commenter commented Jun 6, 2021

Codecov Report

Merging #1653 (ff49396) into master (2b838f4) will not change coverage.
The diff coverage is 0.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1653   +/-   ##
=======================================
  Coverage   73.22%   73.22%           
=======================================
  Files         504      504           
  Lines       59811    59811           
=======================================
  Hits        43794    43794           
  Misses      13537    13537           
  Partials     2480     2480           
Impacted Files Coverage Δ
lapack/gonum/dlasy2.go 88.00% <0.00%> (ø)

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 2b838f4...ff49396. Read the comment docs.

@soypat
Copy link
Contributor Author

soypat commented Jun 9, 2021

Should I close this request and let someone experienced with the function test this? I've tried giving input that's large enough to trigger the condition but it results in errors all over the place. I'm unfamiliar with what it is that is being tested. Here's what I tried:

    // declare tl, tr, b etc. in dlasy2.go in testlapack
     for i := range tl.Data {
		tl.Data[i] = math.Abs(tl.Data[i]) * 1e-10
	}
	if len(b.Data) > 0 {
		b.Data[0] = 1e64
      }

@vladimir-ch
Copy link
Member

Should I close this request and let someone experienced with the function test this?

No, you found the bug and the fix is clear even without having it covered by a test. After you've added yourself to CONTRIBUTORS/AUTHORS, I'll merge this PR and then try to take a look at the test.

@vladimir-ch vladimir-ch changed the title lapack/gonum/dlasy2.go: prevent division of untyped ints always yielding zero lapack/gonum: prevent division of untyped ints always yielding zero in Dlasy2 Jun 11, 2021
@vladimir-ch vladimir-ch requested a review from kortschak June 11, 2021 13:29
@vladimir-ch vladimir-ch merged commit e4c04dd into gonum:master Jun 11, 2021
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.

4 participants