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

MaxBetween panics on a view and scalar #145

Open
aglyzov opened this issue Dec 23, 2024 · 2 comments
Open

MaxBetween panics on a view and scalar #145

aglyzov opened this issue Dec 23, 2024 · 2 comments

Comments

@aglyzov
Copy link

aglyzov commented Dec 23, 2024

Here is a minimal demonstration code:

package main

import (
	"fmt"
	"os"

	"gorgonia.org/tensor"
)

func main() {
	a := tensor.New(
		tensor.WithShape(3, 2),
		tensor.WithBacking([]float64{1, 2, 3, 4, 5, 6}),
	)

	x, err := a.Slice(nil, tensor.S(0))
	if err != nil {
		fmt.Println(err)
		os.Exit(2)
	}

	var scalar = float64(3.5)

	_, err = tensor.MaxBetween(x, scalar, tensor.UseUnsafe()) // <-- this panics: "unreachable"
	if err != nil {
		fmt.Println(err)
		os.Exit(1)
	}
}
@aglyzov
Copy link
Author

aglyzov commented Dec 23, 2024

I think the following patch fixes the issue. Could you please check if I am not missing anything:

diff --git a/defaultengine_minmax.go b/defaultengine_minmax.go
index 56ac432..a8383da 100644
--- a/defaultengine_minmax.go
+++ b/defaultengine_minmax.go
@@ -169,7 +169,7 @@ func (e StdEng) MinBetweenScalar(t Tensor, s interface{}, leftTensor bool, opts
 	}
 
 	// check to see if anything needs to be created
-	if reuse == nil {
+	if safe && reuse == nil {
 		reuse = NewDense(a.Dtype(), a.Shape().Clone(), WithEngine(e))
 		dataReuse = reuse.hdr()
 		if useIter {
@@ -275,7 +275,7 @@ func (e StdEng) MaxBetweenScalar(t Tensor, s interface{}, leftTensor bool, opts
 	}
 
 	// check to see if anything needs to be created
-	if reuse == nil {
+	if safe && reuse == nil {
 		reuse = NewDense(a.Dtype(), a.Shape().Clone(), WithEngine(e))
 		dataReuse = reuse.hdr()
 		if useIter {

@aglyzov
Copy link
Author

aglyzov commented Dec 23, 2024

Explanation of the fix:
The code in {Min,Max}BetweenScalar assumes it should always have a reuse (*Dense) populated:

	// check to see if anything needs to be created
	if reuse == nil {
		reuse = NewDense(a.Dtype(), a.Shape().Clone(), WithEngine(e))
		dataReuse = reuse.hdr()
		if useIter {
			iit = IteratorFromDense(reuse)
		}
	}

Which is not true for the case when there is an unsafe option. If the unsafe option is given then we should output into the first Tensor.

Otherwise the following switch fails to pick the first case:

		switch {
		case !safe && reuse == nil:
			err = e.E.MaxBetweenIter(typ, dataA, dataB, ait, bit)
			retVal = a
		case safe && reuse != nil && !leftTensor:
			storage.CopyIter(typ, dataReuse, dataB, iit, bit)
			bit.Reset()
			iit.Reset()
			err = e.E.MaxBetweenIter(typ, dataA, dataReuse, ait, bit)
			retVal = reuse
		case safe && reuse != nil && leftTensor:
			storage.CopyIter(typ, dataReuse, dataA, iit, ait)
			ait.Reset()
			iit.Reset()
			err = e.E.MaxBetweenIter(typ, dataReuse, dataB, iit, bit)
			retVal = reuse
		default: // safe && bool
			panic("Unreachable")
		}

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

No branches or pull requests

1 participant