Skip to content
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

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

Merged
merged 4 commits into from
Oct 12, 2020

Conversation

BastienTr
Copy link
Collaborator

Commit bcc36dd 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
Copy link
Contributor

eSoares commented Oct 9, 2020

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 a9d3aa7, 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
Copy link
Collaborator Author

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

eSoares commented Oct 9, 2020

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

BastienTr commented Oct 11, 2020

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

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

@coveralls
Copy link

Coverage Status

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

@BastienTr
Copy link
Collaborator Author

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

@eSoares
Copy link
Contributor

eSoares commented Oct 12, 2020

Looks good 👍

Also, thanks for the reference! :-)

@BastienTr BastienTr merged commit ee289ae into veeresht:master Oct 12, 2020
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.

None yet

3 participants