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

289 fib decoder #293

Closed
wants to merge 9 commits into from
Closed

289 fib decoder #293

wants to merge 9 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Nov 24, 2022

Summary

Addresses #289
Fibonacci Decoder

Details and comments

@ghost ghost self-assigned this Nov 24, 2022
@ghost ghost marked this pull request as draft November 24, 2022 00:31
@ghost ghost requested a review from quantumjim November 24, 2022 05:25
@ghost ghost marked this pull request as ready for review November 24, 2022 05:25
@ghost
Copy link
Author

ghost commented Nov 24, 2022

I think something might be wrong with our checks? This should be flagging pylint errors for lack of documentation, no?

@@ -0,0 +1,19 @@
import numpy as np

Copy link
Collaborator

@dsvandet dsvandet Nov 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make sure to include a strong description of what this method is doing and building.

Comments:

  1. Missing documentation stating what the method is creating and a reference to the paper. i.e. Sec II of paper gives the appropriate definitions ... Also state that is this the particular case for L=2^k. Need to include what start_arr is. I might relabel this as start_array or start_vector or simply seed
  2. Missing type hints
  3. I might change dtype=int to dtype=np.int8
  4. I might shorten the name to just gen_fib_code_word
  5. This is only well defined for L=2^k so that should be checked for. It is not clear to me if the input should therefor be k instead of L then. Either way I would create a gen_fib_code_word and _gen_fib_code_word which is consistent with the framework style.

Copy link
Collaborator

@dsvandet dsvandet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the code from Georgia and Ben or is this code that you developed? If code from Georgia and Ben please add to comments that code was originally developed by ... and include a reference to the paper.

Does this code "specialty" code, i.e. not really using the framework but just an inclusion, If so we should discuss how to include such code and an initial set. We have to make the same decisions for the machine learning decoder that we are moving in. Let's discuss.

@grace-harper grace-harper marked this pull request as draft November 30, 2022 22:59
@grace-harper
Copy link
Collaborator

Hey Drew,
The quantum code was developed by Ben and Georgia. The python code, I wrote based on their paper. I have the paper referenced in the README, but I can reference it elsewhere if need be.

@CLAassistant
Copy link

CLAassistant commented Oct 2, 2023

CLA assistant check
All committers have signed the CLA.

…nctionality into ClassicFibDecoder

* ClassicFibDecoder -> ClassicFibonacciDecoder
* Added Documentation
* Pylint/Black Adherence
@grace-harper grace-harper mentioned this pull request Oct 27, 2023
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.

3 participants