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

Add Arabic wordcloud example (in a readme file) See #315 #554

Closed
wants to merge 1 commit into from

Conversation

iamaziz
Copy link

@iamaziz iamaziz commented May 15, 2020

No description provided.

@AMR-KELEG
Copy link
Contributor

AMR-KELEG commented May 16, 2020

Hi @iamaziz ,
Thanks for taking the initiative to finally add the example to the package.
I will share my thoughts regarding this example.

IMHO, we should work on merging the original pull request and give credit to the original author (#315).
OTOH, the current status of the original PR indicates that it needs some minor bug fixes (Renaming filename, adding the font that you have used "which is better IMO" and modifying the long comment messages used there).

For your script:

  • You have added some lines for counting the words which is the only difference to the original script (https://github.com/amueller/word_cloud/blob/cb926bd718f654bd588b2f29c5fcd482961dd01d/examples/arabic.py)
    This is the output of the original script after applying bug fixes.
    arabic_example
    Will using a Counter yield better results?
    I have a feeling that avoiding the Counter relies on a better tokenizer than the one you have used COUNTS = Counter(my_arabic_text.split())
  • A README is always nice to have but I am think that the whole script should be moved to a separate .py file (which is the convention that is currently used in the repository).

@iamaziz
Copy link
Author

iamaziz commented May 16, 2020

Yea definitely agreed all the credit goes to the original PR (and its comments), it's linked in the title of this PR. I just thought it would be nicer (at least) to have an example for a quick how-to reference so people can see and adopt (even as a readme file, after failing the CI #553 with .py file). Instead of muddling in the comments of a three-year-old PR just to figure out a simple step.

This PR is probably not the better direction, please feel free to take over both PRs or close this one :] and let me know if can help

@iamaziz
Copy link
Author

iamaziz commented May 17, 2020

I felt guilty for the state of this PR :/ @AMR-KELEG perhaps it makes sense if we close it, for now, to follow up with the original PR and the example wrapper there.

@iamaziz iamaziz closed this May 17, 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.

2 participants