search

veeresht / CommPy

Digital Communication with Python
http://veeresht.github.com/CommPy
BSD 3-Clause "New" or "Revised" License
551 stars 179 forks source link

[Bug fix] Bad typing in links.py introduced in bcc36ddc #88

Closed BastienTr closed 3 years ago

BastienTr commented 3 years ago

Commit bcc36ddc introduced a typing error in link_performance. Typing received_msg as int prevent soft detectors to work properly as LLRs are converted to integers. I can provide a minimal example for this but any test with soft detectors shows the error.

@eSoares: Since you hadded this, could you confirm that this is not need required so that I merge this PR before v0.6 release.

eSoares commented 3 years ago

Before going forward with this merge I have a couple of issues.

First it points an error that currently no test exists to catch it, maybe a test case should be also added.

Second, if this is merged, the bitwise_xor will fail (travis already said the same). This could be solve by rolling back commit a9d3aa7958798d52e814145af6290e611fbc8eca, but has a performance hit. An alternative would be to check the return of the decode, and if int then use bitwise operation (shouldn't always be int?).

BastienTr commented 3 years ago

I have a look this weekend. I agree that we may need a new testcase.

We have to fix this anyway. Performance is cool but if the result is false for sort detectors, it's not acceptable. We may find a way to have both.

eSoares commented 3 years ago

We have to fix this anyway. Performance is cool but if the result is false for sort detectors, it's not acceptable. We may find a way to have both.

Completely agree! I was suggesting having a "fast path" when possible.

Also, I'm not well verse in the the topic of MIMO decoding and soft detectors, sorry for this bug, completely flew bellow my radar. If you could drop a reference as a pointer would greatly appreciate!

BastienTr commented 3 years ago

I push the modified code as soon as all tests pass on my laptop.

First it points an error that currently no test exists to catch it, maybe a test case should be also added.

Done. I refactor test_link_performance to avoid code duplication while adding the new testcases. I also reduces the number of iterations to speedup the computation. Some tests show that there are enough precision for the specified relative tolerance.

Second, if this is merged, the bitwise_xor will fail (travis already said the same). This could be solve by rolling back commit a9d3aa7, but has a performance hit.

You were right to introduce bitwise_xor since BERs are computed on bits. However, introducing the typing in received_msg is not a good idea since you can either receive bits (integers) or LLRs (floats). The solution was to convert decoded_bits to integers.

Also, I'm not well verse in the the topic of MIMO decoding and soft detectors, sorry for this bug, completely flew bellow my radar. If you could drop a reference as a pointer would greatly appreciate!

On MIMO detection, I recommend this really good state of the art that includes soft detection: Fifty Years of MIMO Detection.

BastienTr commented 3 years ago

Does someone know what is the default working directory when Travis executes the tests?

coveralls commented 3 years ago

Coverage Status

Coverage increased (+3.08%) to 82.353% when pulling fa21b2a0471d9017a2f027e1052f533ad569b606 on BastienTr:master into 292b4e584e2f0ee4f2adf3e41ec67ff24210a844 on veeresht:master.

BastienTr commented 3 years ago

@eSoares, do you commit that this is OK for you?

eSoares commented 3 years ago

Looks good :+1:

Also, thanks for the reference! :-)