-
-
Notifications
You must be signed in to change notification settings - Fork 359
Implement huffman encoding in ocaml #750
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
Implement huffman encoding in ocaml #750
Conversation
Just checked, the codebook generated by this tree is also totally valid! |
Is there something wrong with the implementation? It was merged, right? |
I think this PR is ready for review; however, I don't have ocaml expertise, so if someone else could step in that would be great! |
[lang: ocaml] |
Hum... now that I look at it, you need to insert the code in the chapter (markdown) file before we can merge @shubhamkumar13 |
If the code is alright, we can probably add the line numbers in and merge this soon? I wasn't sure if the comment from @Amaras meant the code looked good or not |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Globally it looks good and it works. A few things seem to be a bit more complicated than it should be, I've addressed a couple of them.
I would suggest to also create a separate file for the priority queue (and perhaps for some of the helper functions), since it's not the main point of the chapter, it would make the example more readable. But I don't mind it being all in the same file.
@ShadowMitia thanks for reviewing this I've left a comment related to the I'll try and replace append function usage, I was also curious how you were able to suggest changes in comments I want to try it, it seems a great way to pinpoint changes. Thanks again! |
@shubhamkumar13 Glad I can help! For suggestions in github I learned it from here. |
Co-authored-by: Dimitri Belopopsky <[email protected]>
Co-authored-by: Dimitri Belopopsky <[email protected]>
Co-authored-by: Dimitri Belopopsky <[email protected]>
Co-authored-by: Dimitri Belopopsky <[email protected]>
Co-authored-by: Dimitri Belopopsky <[email protected]>
Co-authored-by: Dimitri Belopopsky <[email protected]>
Co-authored-by: Dimitri Belopopsky <[email protected]>
Co-authored-by: Dimitri Belopopsky <[email protected]>
Co-authored-by: Dimitri Belopopsky <[email protected]>
c388248
to
95174f3
Compare
@ShadowMitia I've updated the edits let me know if this looks good, Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks good now. Thanks for the work!
References -> https://ocaml.org/learn/tutorials/99problems.html