locate() does not implement borrow check?

[polarathene]

See my comments on this issue. I've provided a test sample to demonstrate slight change in when locate is used that provides an incorrect result or triggers a not so helpful panic depending on dims used. I've been told that making use of the borrow checker can inform users at compile time when the API is used incorrectly.

Based on other errors/discrepencies with other AF functions causing confusion for me, I think many other functions would benefit from this?

[9prady9]

@polarathene I think I found the problem. The bug was in indexing code and not in af::locate.

Using the below code

extern crate arrayfire as af;
use af::*;


fn main() {

    let dims   = af::Dim4::new(&[1, 3, 1, 1]);
    let bools  = af::Array::new(&[1, 0, 1], dims);
    let values = af::Array::new(&[2, 5, 6], dims);

    print( &bools );
    print( &filter_edited(&values, &bools) );
    print( &af::locate(&bools) );
    print( &filter_edited(&values, &af::locate(&bools)) );

    print( &filter_original(&values, &bools) );

}

fn filter_edited(array: &af::Array, bools: &af::Array) -> af::Array {
    let mut idxr = af::Indexer::new();
    idxr.set_index(bools, 0, None);
    af::index_gen(array, idxr)
}

fn filter_original(array: &af::Array, bools: &af::Array) -> af::Array {
    let mut idxr = af::Indexer::new();
    idxr.set_index(&af::locate(bools), 0, None);
    af::index_gen(array, idxr)
}

I don't get the below corrupted output for filter_original after the fix.

[1 3 1 1]
         1          0          1 

[3 1 1 1]
         5 
         2 
         5 


[2 1 1 1]
         0 
         2 


[2 1 1 1]
         2 
         6 


[3 1 1 1] - // This doesn't happen anymore, it was due to incorrect reference count of the arrays used in indexing. 
         6 
         2 
         6 

After the fix, the following code gives expected output.

fn filter_original(array: &af::Array, bools: &af::Array) -> af::Array {
    let mut idxr = af::Indexer::new();
    idxr.set_index(&af::locate(bools), 1, None);  
// Note that indexing dimension is not 0, and input is 2d [1 3 1 1], unlike [3 1 1 1] which is treated as one dimensional by ArrayFire.
    af::index_gen(array, idxr)
}

[1 2 1 1]
         2          6 

fn filter_original(array: &af::Array, bools: &af::Array) -> af::Array {
    let mut idxr = af::Indexer::new();
    idxr.set_index(&af::locate(bools), 0, None);
    af::index_gen(array, idxr)
}

[2 3 1 1]
         2          5          6 
         2          5          6 

[polarathene]

That's interesting, my current version is:

fn filter_array(array: &af::Array, bools: &af::Array) -> af::Array {
    let mut idxr = af::Indexer::new();
    let indices = &af::locate(bools); // Assigning a variable first was required
    idxr.set_index(indices, 0, None);

   &af::index_gen(array, idxr)
}

Which is similar to what my edited version I shared earlier did, not sure why the behaviour was different. Was it due to set_index with 0 instead of 1 for dimension on a [1,3,1,1] array? Oh I see the output shifted the dimension, I thought this was expected due to a new array.

[9prady9]

What do you mean by shifted the dimension ?

May be I confused you, I shall try to explain in detail once again. The dimension with which set_index is called will have an effect on how the output looks like. In this case, input array is considered 2D because there are > 1 elements along second dimension. The indexing operations will repeat elements if index array size is greater than the number of elements of array along given dimension. Lets take the example we have used to debug this issue.

set_index is called with 0 as dim and input is of dims [1 3 1 1]. Since the input array has only one element along first(0th) dimension, for every index along that dimension, the only existing element is repeated because all indices point to the same element. Thus we see the following output

[2 3 1 1]
         2          5          6 
         2          5          6 

Also, let indices = &af::locate(bools); // Assigning a variable first was required that assignment was needed because of the bug which is fixed now - related to reference count of arrays used in indexing.

[polarathene]

Yes, I think we're on the same page there. Prior [1,3,1,1] as reported when set_index was applied, the array returned was [3,1,1,1]. This isn't what was meant to happen according to info here, and instead the result you shared should have happened unless the correct dimension was given to set_index to return [1,2,1,1] (not [2,1,1,1]) with elements [2,6] instead.

[polarathene]

I ran into the issue I described with locate on another function. Where my solution above does:

let indices = &af::locate(bools); // Assigning a variable first was required
idxr.set_index(indices, 0, None);

Instead of the original that did:

idxr.set_index(&af::locate(bools), 0, None);

Which caused problems described in the other thread. You can see that instead of assigning to a variable locate() call was used as a paramater that was passed to set_index(), it was the one difference of inlining the locate() call in set_index() that caused the different output.

--

I just tried out the join_many! macro, and had an error, so I tried the example which worked fine. I noticed the same behavior here:

let c = &randu::<f32>(Dim4::new(&[5, 3, 1, 1]));
let d = join_many![2; a, b, c];

As in the example works but apparently this is not the same?

let c = randu::<f32>(Dim4::new(&[5, 3, 1, 1]));
let d = join_many![2; a, b, &c];

randu() in the docs states it returns an Array like any other. Perhaps this is a subtle difference in Rust that I'm not aware of, or something else is going on.

I know that you have apparently fixed this due to indexing, but is that fix addressing what this issue was referring to specifically?

I've been told that making use of the borrow checker can inform users at compile time when the API is used incorrectly.

Rather than the dims, this subtle change might behave diferently and the borrow checker might be able to pick up on it was a suggestion a user made when I first ran into the issue with locate(). Rather than a panic I get a helpful error: error: expected ident, found &, with the error pointing at the &c variable in join_many![2; a, b, &c];

When I check type with c = (); I get &af::Array, with let c = randu instead of let c = &randu I will get the same type with &c = (); or with c = (); as expected: ``af::Array`. The type should be correct? yet for some reason it causes a compile error?

[polarathene]

Avoiding the join_many! macro and using the join_many() method directly with a vector, it doesn't matter if I push &c or c variant into the vector, both work fine. Perhaps this time the issue is due to the macro?

[9prady9]

@polarathene I shall look into the macro. Thank you once again for reporting it.

[9prady9]

@polarathene The error error: expected ident, found & indicates one can't pass a reference of object to parameter marked as ident in macro definition. &<name> is not considered an ident but an expr. Once I changed the macro parameter as expr, the code where you pass &c instead c worked just fine. This has nothing to do with the problem you were seeing earlier with set_index.

[polarathene]

@9prady9 Good to know, another bug fixed :)

[9prady9]

I am not sure if expr is better or ident is better. One thing is sure, this is not a bug, neither in ArrayFire nor Rust. May be the macro can be changed to use expr but I would have to think about it.

[polarathene]

I am used to creating my arrays as shown elsewhere not with the variable assignment of the call being referenced. When I use AF methods I add the reference. The af_print! macro does not seem to require reference. Like stated c as in the example appeaers to be the equivalent of &c macro aside, it breaks consistency a bit.

If you'd like an experienced opinion the guys on #rust-beginners IRC channel are very helpful and knowledgeable. Some are Rustlang devs and could advise what might be best :)

---

The examples also tend to avoid assigning Arrays like the join_many! macro example does. If it is preferrable to create Arrays like shown in join_many! should these examples reflect that or should docs explain when to prefer such an assignment?

[9prady9]

I have created an issue for improvement of macros, #144, you can track the progress of it over there.