Skip to content

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

Merged

Conversation

shubhamkumar13
Copy link
Contributor

@shubhamkumar13
Copy link
Contributor Author

huffman code

This might be the one

@leios
Copy link
Member

leios commented Sep 6, 2020

Just checked, the codebook generated by this tree is also totally valid!

@shubhamkumar13
Copy link
Contributor Author

shubhamkumar13 commented Sep 10, 2020

Is there something wrong with the implementation? It was merged, right?

@berquist berquist added the Implementation This provides an implementation for an algorithm. (Code and maybe md files are edited.) label Oct 3, 2020
@leios
Copy link
Member

leios commented Oct 12, 2020

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!

@ntindle
Copy link
Member

ntindle commented Aug 28, 2021

[lang: ocaml]

@Amaras
Copy link
Member

Amaras commented Aug 28, 2021

Hum... now that I look at it, you need to insert the code in the chapter (markdown) file before we can merge @shubhamkumar13
I don't really have expertise in OCaml, sadly, so I can't review this PR properly.

@leios
Copy link
Member

leios commented Sep 15, 2021

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

Copy link
Contributor

@ShadowMitia ShadowMitia left a 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.

@shubhamkumar13
Copy link
Contributor Author

@ShadowMitia thanks for reviewing this I've left a comment related to the @@ operator, I think I can remove it since it doesn't matter much either way.

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!

@ShadowMitia
Copy link
Contributor

@shubhamkumar13 Glad I can help!

For suggestions in github I learned it from here.

@shubhamkumar13
Copy link
Contributor Author

@ShadowMitia I've updated the edits let me know if this looks good, Thanks!

Copy link
Contributor

@ShadowMitia ShadowMitia left a 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!

@ShadowMitia ShadowMitia merged commit 03d21df into algorithm-archivists:main Mar 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Implementation This provides an implementation for an algorithm. (Code and maybe md files are edited.) lang: ocaml
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants