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.

[9prady9]

Closing this issue as locate doesn't have issues as suspected earlier. Please follow the indexing issue progress on the #149.

[9prady9]

@polarathene I have put in a fix for this in devel just now. Can you please test it on your end and let me know if everything is working fine. The borrow check will complain now if you are using temporarily allocated Arrays to call set_index.

[polarathene]

@9prady9 Compiling with latest devel with this function in my src:

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

    join(0, &af::index_gen(array, idxr), &indices)
}

Error:

error: borrowed value does not live long enough
   --> src/main.rs:551:1
    |
547 |     let indices = &af::locate(bools);
    |                    ----------------- temporary value created here
...
551 | }
    | ^ temporary value dropped here while still borrowed
    |
    = note: values in a scope are dropped in the opposite order they are created

So my "fix" earlier was incorrect? :P Or was this unintended?

[polarathene]

Compiler is happy with this change:

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

    join(0, &af::index_gen(array, idxr), &indices)
}

[polarathene]

It's also happy if I change the set_index line to idxr.set_index(&af::locate(bools), 0, None); So I guess my new error was about indices on join()?(it probably shouldn't have been && whoops) I haven't tried running the function.

[9prady9]

@polarathene That's odd idxr.set_index(&af::locate(bools), 0, None); that should throw an error. It should not work after the fix. The fix is to force the user to not pass Array objects created in such manner.

Edited:
The reason that shouldn't be allowed is that set_index is only assigning internal af_array handle of Array to af_indexer_t object of C API. If temporary Array objects are allowed, they would get dropped once that line of expression is done executing. Therefore, creating the undefined results which we were seeing earlier.

[9prady9]

fyi - 8893683 should be the hash of latest devel HEAD.

[polarathene]

idxr.set_index(&af::locate(bools), 0, None); was the way the original method was written on the Steam Compaction issue. That didn't throw an error in the past, just caused a panic in my test. I've not tested the logic, presumably it's correct now like you mentioned earlier.

let indices = &af::locate(bools); caused an error because of the join() call at the end. It actually picked up a bug, I already had a reference but referenced it again so indices was &&.

I can confirm I'm on that commit.

[9prady9]

This is what I get on my machine if we do idxr.set_index(&af::locate(bools), 0, None);

error: borrowed value does not live long enough
  --> examples/helloworld.rs:28:48
   |
28 |     idxr.set_index(&af::locate(bools), 0, None);
   |                     -----------------          ^ temporary value dropped here while still borrowed
   |                     |
   |                     temporary value created here
29 |     af::index_gen(array, idxr)
30 | }
   | - temporary value needs to live until here
   |
   = note: consider using a `let` binding to increase its lifetime

error: aborting due to previous error

True, that was the initial way in which the code stub in stream compaction was implemented. But, that is a buggy code even though it worked in few cases earlier.

[polarathene]

Oh the latest commit should be throwing an error for that? Maybe I need to do a cargo clean, hold on.

[polarathene]

Cargo clean seemed to fix that.

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();
    let indices = &af::locate(bools);
    idxr.set_index(indices, 0, None);
    af::index_gen(array, idxr)
}

Errors:

error: `indices` does not live long enough
   --> src/main.rs:551:1
    |
548 |     idxr.set_index(&indices, 0, None);
    |                     ------- borrow occurs here
...
551 | }
    | ^ `indices` dropped here while still borrowed
    |
    = note: values in a scope are dropped in the opposite order they are created

error: borrowed value does not live long enough
   --> src/main.rs:605:1
    |
602 |     let indices = &af::locate(bools);
    |                    ----------------- temporary value created here
...
605 | }
    | ^ temporary value dropped here while still borrowed
    |
    = note: values in a scope are dropped in the opposite order they are created

error: aborting due to 2 previous errors

[polarathene]

1st errror was referring to this:

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

    join(0, &af::index_gen(array, idxr), &indices)
}

2nd to filter_original()

[9prady9]

You have to move let indices = af::locate(bools); before let mut idxr = af::Indexer::new(); because the objects are dropped in the reverse order of creation. Since idxr is borrowing a reference of indices, we can't drop indices before idxr - this is order is chosen because of the order in which we created those variables.

[9prady9]

@polarathene Is it clear now why idxr.set_index(&af::locate(bools), 0, None); should not be allowed - thus the current devel HEAD is throwing error for it ?

[polarathene]

@9prady9 Thanks that fixed it, I was just about to ask how I was meant to use the method now :P First time I've seen the order of declaration matter like that.

I'm still reasonably fresh to Rust, so I don't quite grasp the ownership/borrowing/lifetimes fully yet. I can see that tripping up other users who might not be experienced enough with Rust, though an example snippet with the correct order would probably get copy/pasted or could be noticed on a tutorial/example about indexing with the Rust wrapper :)

The note is good, I didn't actually read it until just now ha. And yes I get why now sort of, in the sense that &af::locate(bools) isn't above let mut idxr = af::Indexer::new(); so it will cause a problem being dropped first due to idxr using it still, it needs to be dropped first to "free" the locate()/indices reference.

Definitely a helpful note to include in docs/faq somewhere :)

[9prady9]

@polarathene The business of borrow checking references is surely confusing for a while in the beginning. I took me a while to understand that lifetime of references in set_index call are the reason why borrow checker did not complain earlier. However, the rust compiler's error messages is what guided me in fixing this problem. I guess this order of creation plays a significant role when objects and references come into play. For integral types where copy trait is implicitly defined, this won't be an issue at all.

While I was at fixing this bug, I used the code you shared with me to debug the problem to write a basic example in the documentation for Indexer struct. Will add more examples once I have time to work on it.