Issue with wrapping

[MaenMallah]

Hello again,

As I spent some time debugging an issue and finally found the reason, I thought I can let you know with my potential solution for it.

Wrapping is not working when values are negative.

How to replicate:

x = Fxp(0, signed=True, n_word = 4, n_frac = 0)

x.equal(-30) ==> -14 which is not possible as we can only represent the range [-8,7]
This happens because the value I gave should wrap twice but the functions don't account for this. The positive case is correct.

To solve this I changed the wrap function inside utils.py

def wrap(x, val_min, val_max, signed, n_word):
    if not ((x <= val_max) & (x >= val_min)):
        if signed:
            x_wrapped = x_wrapped = twos_complement_repr(x, n_word)
            while x_wrapped < val_min:
                x_wrapped = twos_complement_repr(x_wrapped, n_word)
        else:
            x_wrapped = x % (1 << n_word)
    else:
        x_wrapped = x
    return x_wrapped

However, I am not sure if I broke something else.

[francof2a]

Mello @MaenMallah

Yes, you're right. When I tested it I didn't cover range with several wraps around limits. Thank you for the time you invest in this!

Your solution seems to work well, but it has problems with performance. All of this show me that the original implementation also have this kind of problem (beside it is not working as it should) and use twos complement could be a bad idea.

I have been thinking about an alternative and reach to this:

def wrap(x, signed, n_word): 
    m = (1 << n_word)
    if signed: 
        x = np.array(x).astype(int) & (m - 1) 
        x = np.where(x < (1 << (n_word-1)), x, x | (-m)) 
    else: 
        x = np.array(x).astype(int) & (m - 1) 
    return x

I tested it and work well!

About performance (wrap0 is original function, wrap2 is your solution):

In [ ]: v = np.arange(-32,32)                                                                                                                                                                        

In [ ]: w = np.arange(-8192,8192) 

In [ ]: %timeit wrap(v, True, 3)                                                                                                                                                                    
3.4 µs ± 21.6 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)

In [ ]: %timeit np.vectorize(wrap0)(v, -4, 3, True, 3)                                                                                                                                              
65.7 µs ± 490 ns per loop (mean ± std. dev. of 7 runs, 10000 loops each)

In [ ]: %timeit np.vectorize(wrap2)(v, -4, 3, True, 3)                                                                                                                                              
88 µs ± 417 ns per loop (mean ± std. dev. of 7 runs, 10000 loops each)

In [ ]: %timeit wrap(w, True, 3)                                                                                                                                                                    
37.1 µs ± 395 ns per loop (mean ± std. dev. of 7 runs, 10000 loops each)

In [ ]: %timeit np.vectorize(wrap0)(w, -4, 3, True, 3)                                                                                                                                              
10.8 ms ± 90.3 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)

In [ ]: %timeit np.vectorize(wrap2)(w, -4, 3, True, 3)                                                                                                                                              
1.81 s ± 5.1 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

So, now we have an even better wrap function :) Thanks again!

---

As you could be noted, the arguments val_min and val_max are not necessary. If you use this new wrap function an extra change in objects.py is necessary:

# val = utils.wrap(new_val, val_min, val_max, self.signed, self.n_word)
val = utils.wrap(new_val, self.signed, self.n_word)

It would be great if you can test it and confirm is working well.
I plan to update the repo if all is working well.

[MaenMallah]

Hello @francof2a , I have tested the new implementation on the git and the use-case I had works fine now. All seems to be working well. Thanks for your quick replies and fixes.