scijs / ndarray-ops

ndarray operations
MIT License
66 stars 14 forks source link

Advice needed for nullargmin/nullargmax #7

Closed letmaik closed 9 years ago

letmaik commented 9 years ago

Following the discussion on scijs/ndarray#22 I tried implementing this, mostly by copy pasting the existing argmin/max:

https://github.com/Reading-eScience-Centre/leaflet-coverage/blob/master/test/util/ndarray-ops-null.spec.js

For reference:

function nullargminmax (op) {
  let minus = op === 'max' ? '-' : ''
  let comp = op === 'max' ? '>' : '<'

  // adapted from ndarray-ops argmin/argmax
  return compile({
    args:["index","array","shape"],
    pre:{
    body:"{this_v=" + minus + "Infinity;this_i=_inline_0_arg2_.slice(0)}",
    args:[
      {name:"_inline_0_arg0_",lvalue:false,rvalue:false,count:0},
      {name:"_inline_0_arg1_",lvalue:false,rvalue:false,count:0},
      {name:"_inline_0_arg2_",lvalue:false,rvalue:true,count:1}
      ],
    thisVars:["this_i","this_v"],
    localVars:[]},
    body:{
    body:"{if(_inline_1_arg1_ !== null && _inline_1_arg1_ " + comp + "this_v){this_v=_inline_1_arg1_;for(var _inline_1_k=0;_inline_1_k<_inline_1_arg0_.length;++_inline_1_k){this_i[_inline_1_k]=_inline_1_arg0_[_inline_1_k]}}}",
    args:[
      {name:"_inline_1_arg0_",lvalue:false,rvalue:true,count:2},
      {name:"_inline_1_arg1_",lvalue:false,rvalue:true,count:2}],
    thisVars:["this_i","this_v"],
    localVars:["_inline_1_k"]},
    post:{
    body:"{return this_i}",
    args:[],
    thisVars:["this_i"],
    localVars:[]}
  })
}
var nullargmin = nullargminmax('min')
var nullargmax = nullargminmax('max')

It works. However, I have a problem when the array contains only nulls. Then for some reason I get an array [2] as an answer where the number is the length of the array, e.g. for ndarray([null,null]). Does this make any sense? I'm a bit lost understanding this low-level code.

rreusser commented 9 years ago

The .slice in the pre statement exists to get a new array of the same size as the dimensions. Your issue is that it's a duplicate and isn't initialized. If you have all nulls and want nulls as the answer, try this:

body:"{this_v=" + minus + "Infinity;this_i=_inline_0_arg2_.slice(0);for(var _inline_1_k=0;_inline_1_k<this_i.length;_inline_1_k++){this_i[_inline_1_k]=null}}",

It just initializes the answer (this_i) to contain all nulls. As soon as any non-null entry is encountered, it receives the answer. I get what I think would be expected:

nullargmin(ndarray([null,null,1,null],[2,2]))
// => [ 1, 0 ]

nullargmin(ndarray([null,null,null,null],[2,2]))
// => [ null, null ]
rreusser commented 9 years ago

(Also, this would be way easier if not editing the compiled versions. I think ndarray-ops uses the compiled version just to skip that step on common operations.)

letmaik commented 9 years ago

It complains because i is undefined in your snippet. Also, I don't understand why your first example just return [1,0]. Can you explain?

rreusser commented 9 years ago

Oops. Sorry. I switched it at the last moment to look like cwise variable names. One second.

Ricky Reusser

On Aug 26, 2015, at 5:11 PM, Maik Riechert notifications@github.com wrote:

It complains because i is undefined in your snippet. Also, I don't understand why your first example just return [1,0]. Can you explain?

— Reply to this email directly or view it on GitHub.

rreusser commented 9 years ago

Updated. That was a typo on my part. sorry about that.

As for the answer, argmin is just the index of the smallest element in the array. If there are two dimensions, as there are in my example, then you need two indices to locate the element in the array ((1,0), in my example).

rreusser commented 9 years ago

(It's possible to write an operator that returns the index (argument) and the value of the min/max element, but that's not what this does. It returns just the index. n-dimensions = n components to the answer.)

letmaik commented 9 years ago

Thanks for the help. All working and understood!

letmaik commented 9 years ago

Actually, I changed the post body to body:"{return this_i[0] === null ? null : this_i}", I find it more useful to return a plain null in that case. You cannot use a null array for get() anyway, and checking if this case happens is easier with a plain null, just opsargmin(arr) === null instead of opsargmin(arr)[0] === null.

rreusser commented 9 years ago

Makes sense. Great!