Skip to content

Faster twos_comp #16

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Aug 3, 2022
Merged

Faster twos_comp #16

merged 8 commits into from
Aug 3, 2022

Conversation

Ajstros
Copy link
Owner

@Ajstros Ajstros commented Aug 2, 2022

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.

@Ajstros Ajstros requested a review from lucask07 August 2, 2022 15:57
@lucask07
Copy link
Collaborator

lucask07 commented Aug 2, 2022

@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
Copy link
Owner Author

Ajstros commented Aug 2, 2022

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
Copy link
Collaborator

lucask07 commented Aug 2, 2022

@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
Copy link
Owner Author

Ajstros commented Aug 3, 2022

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
Copy link
Collaborator

lucask07 commented Aug 3, 2022

@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 bitwise_xor.

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
Copy link
Owner Author

Ajstros commented Aug 3, 2022

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.

@Ajstros Ajstros merged commit 3e5b051 into main Aug 3, 2022
@Ajstros Ajstros deleted the data_to_names_speedup branch August 3, 2022 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants