Math.argmax throwing null pointer exception for my setting.

[cosmir17]

Hi,

I am using Math.argmax and it seems throwing null pointer exception for my setting.

import org.platanios.tensorflow.api.tensors.ops.Math._
val indexNext = argmax[Float, Int](myMatrix: Tensor[Float], null).scalar.toInt

I got the following error:
java.lang.NullPointerException: null
at org.platanios.tensorflow.api.tensors.ops.Math.argmax(Math.scala:1295)
at org.platanios.tensorflow.api.tensors.ops.Math.argmax$(Math.scala:1289)
at org.platanios.tensorflow.api.tensors.ops.Math$.argmax(Math.scala:2102)
at org.platanios.tensorflow.api.tensors.ops.Math.argmax(Math.scala:1278)
at org.platanios.tensorflow.api.tensors.ops.Math.argmax$(Math.scala:1274)
at org.platanios.tensorflow.api.tensors.ops.Math$.argmax(Math.scala:2102)`

'null' value should work as specified in the scala doc :
* @param axes Integer tensor containing the axes to reduce. Ifnull, then all axes are reduced.

I don't know why null pointer exception is seen.

*println(myMatrix) printed on console : Tensor[Float, [1, 3]]

[cosmir17]

I looked at the argmax source code..

  def argmax[T: TF : IsNotQuantized, I: TF : IsIntOrLong, IR: TF : IsIntOrLong](
      input: Tensor[T],
      axes: Tensor[I],
      outputDataType: DataType[IR]
  ): Tensor[IR] = {
    Tensor.fromNativeHandle[IR](NativeTensorOpsMath.argMax(
      executionContext.value.nativeHandle, input.nativeHandle, axes.nativeHandle, outputDataType.cValue))
  }

is it because axes.nativeHandle?
I presume, it may attempt to do, null.nativehandle . .. hence the error

[DirkToewe]

I believe this is more of an error in the documentation than in the implementation, compare Python Docs. ArgMax along multiple axes would be somewhat confusing as You would have to return a Tensor of shape [...reducedShape, nAxes] instead of [...reducedShape].

PR#123 initially included an implementation of argMax that would support axes=null as input. But now I'm not so sure if that's such a good idea. Maybe introducing a new method like argMaxND would be better?

[cosmir17]

@DirkToewe I thought so. 'axes.nativeHandle' looks like not being able to handle 'null'. It seems contrary to the documentation. I think it was a technical debt.

By the way, my 'argmax' function was numpy's. I was trying to do a similar thing to the red circled practice.
https://docs.scipy.org/doc/numpy-1.15.0/reference/generated/numpy.argmax.html

[DirkToewe]

Here You can see how I implemented it in PR123. The idea would be to simply flatten the Tensor and then call argmax:

{ input reshape Shape(-1) }.argmax[INT32](0)

[eaplatanios]

@cosmir17 @DirkToewe One question is whether we want to have that behavior or whether it is counterintuitive. Also, it can always be achieved with a reshape to flatten followed by an argmax, which in my opinion has better semantics. What do you think?

[cosmir17]

@eaplatanios @DirkToewe, I am happy to do the following as DirkToewe demonstrated
val indexNext = myMatrix.reshape(Shape(-1)).argmax(0).scalar.toInt

Nonetheless, either the doc or the implementation can be changed in future PR.
The master branch doesn't have the PR123(null handling) change.

[DirkToewe]

... One question is whether we want to have that behavior or whether it is counterintuitive...

I couldn't agree more. My vote would be leaving the implementation as is if only for the reason that people coming from Tensorflow4Python have an easier time to adapt.

One thing that might be useful in general is a Tensor::flat function. tensor.flat.argmax(0) would be more concise and more readable and it might be useful in many other situations as well.

If there is more people like me who need to find a maximum nd-index, it would probably be better to introduce a new method called argMaxND or something like that instead.

The master branch doesn't have the PR123(null handling) change.

No it doesn't. After @eaplatanios brought me to reason, I removed it from the PR. The PR was about a memory leak that was completely unrelated to argmax. I could have created another PR, but I had second thoughts about whether or not argmax should support axes=null at all.

[cosmir17]

If you guys decide to have a function, I think it would be a better naming to have 'flatten' as normally used in Scala.
tensor.flatten.argmax(0)

[cosmir17]

@DirkToewe I wasn't talking about the PR itself. I was specific about the 'null' handling change. I used a parenthesis (null handling)

[DirkToewe]

Not sure if I know which change You are referring to. The change to the null handling in argmax never made it to the master branch as I have never submitted another PR for that change specifically.

[cosmir17]

@DirkToewe, the following change,

if( null == axes )
{... }
else Tensor.fromNativeHandle[IR](NativeTensorOpsMath.argMax(
executionContext.value.nativeHandle, input.nativeHandle, axes.nativeHandle, outputDataType.cValue))

https://github.com/DirkToewe/tensorflow_scala/blob/3fc5ee2058a358ff4a4c91eae261abf78c12ff51/api/src/main/scala/org/platanios/tensorflow/api/tensors/ops/Math.scala#L1207-L1236

Without the above code, argmax function will throw null pointer exception when 'axes' has 'null' value.

It wasn't pushed to Master as you pointed out 'PR123 wasn't merged'.

I was saying, either the doc(describing null input case) or the method better to be changed. Otherwise, there will be another issue ticket concerning this null case.

[DirkToewe]

I was saying, either the doc(describing null input case) or the method better to be changed. Otherwise, there will be another issue ticket concerning this null case.

You're absolutely right. My vote would be on changing/fixing the documentation.

[cosmir17]

I let you guys (will) decide.

[eaplatanios]

@cosmir17 @DirkToewe I updated the documentation and it will be included in the next release. Thanks for catching this! :)