Ajstros / pyripherals

Python solution for communicating with peripheral ICs
GNU General Public License v3.0
2 stars 3 forks source link

Faster twos_comp #16

Closed Ajstros closed 2 years ago

Ajstros commented 2 years ago

Recently, we added binary_twos_comp in the pyripherals.utils module. We already had twos_comp, but this was meant to be a bit clearer and perform the complete binary two's complement usable going to or from twos_complement form. It also takes advantage of numpy arrays rather than the for loops found in twos_comp. This makes binary_twos_comp clearer, faster, and more generally applicable than twos_comp. For that reason, we are replacing twos_comp with binary_twos_comp, now renamed to twos_comp.

lucask07 commented 2 years ago

@Ajstros Glad to see a faster implementation. Could you please add a few examples with the resulting return values to the docstring?

I tried the new twos_comp in an iPython console and am not getting negative return values.

In [25]: twos_comp([0x8000],16)
Out[25]: array([32768])

I would expect -16384

The previous function may have been poorly named. This function took unsigned register values represented in twos-complement and converted to signed integers.

Ajstros commented 2 years ago

This function took unsigned register values represented in twos-complement and converted to signed integers.

@lucask07 Right, I remember now. It seems like there are two possible things we might want to do that involve two's complement:

  1. Start with a 32-bit signed python int, convert to N-bit two's complement form (the binary_twos_comp does this).
  2. Start with an N-bit two's complement form, convert to 32-bit signed python int (the old twos_comp does this).

I think it would clear things up if we created separate functions for these. Something like int_to_custom_signed for 1 and custom_signed_to_int for 2. That way, we cover all the functionality we want and clear up the naming since neither twos_comp nor binary_twos_comp really only does two's complement. We can also implement both functions with numpy arrays to get the speedup we are looking for. Here's an example of what I'm imagining.

>>>int_to_custom_signed([-5, 5], 16)    # Same as binary_twos_comp([-5, 5], 16)
array([65531, 5])
>>>custom_signed_to_int([65531, 5], 16)    # Same as twos_comp([65531, 5], 16)
array([-5,  5])
lucask07 commented 2 years ago

@Ajstros sounds like a plan -- go for it. I can't think of places where we are using an operation like int_to_custom_signed. Certainly, most usage is custom_signed_to_int to convert ADC values to integers.

Ajstros commented 2 years ago

I can't think of places where we are using an operation like int_to_custom_signed. Certainly, most usage is custom_signed_to_int to convert ADC values to integers.

@lucask07 I agree, although I am using int_to_custom_signed for from_voltage, every other twos_comp usage seemed to be replaceable with custom_signed_to_int. I have swapped everything out now and tested it, passing both the pyripherals tests and the covg_fpga repo tests. I also made sure the record.py functionality from the covg_fpga repo is still working. Could you take a look at these new changes?

lucask07 commented 2 years ago

@Ajstros Docstring and comments to update and then I think this could be merged.

The docstring for custom_signed_to_int includes: "Invert all bits (in num_bits), add 1 (only keeping num_bits)."

The comments of custom_signed_to_int note "Use bitwise_or to invert bits". This should be _bitwisexor.

int_to_custom_signed seems to work but it is not clear how, especially when the input is a negative number. Is there any documentation on how numpy.bitwise_and works with a negative number?

Ajstros commented 2 years ago

int_to_custom_signed seems to work but it is not clear how, especially when the input is a negative number. Is there any documentation on how numpy.bitwise_and works with a negative number?

As far as I can tell np.bitwise_and works as a true bitwise and on the two's complement representation of an int. This stackoverflow post mentions the CPython code which has this comment talking about how

Bitwise operations for negative numbers operate as though on a two's complement representation.

The Numpy documentation for np.bitwise_and says,

Computes the bit-wise AND of the underlying binary representation of the integers in the input arrays.

so it seems the CPython two's complement form of the int is used when dealing with a negative number.