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

Update docstring for hero.wordcloud #102

Closed
vidyap-xgboost opened this issue Jul 17, 2020 · 11 comments
Closed

Update docstring for hero.wordcloud #102

vidyap-xgboost opened this issue Jul 17, 2020 · 11 comments
Assignees
Labels
documentation Improvements or additions to documentation good first issue Good for newcomers

Comments

@vidyap-xgboost
Copy link
Contributor

vidyap-xgboost commented Jul 17, 2020

After the discussion on #78

We should add something like:

"To reduce blur in the images, width and height should have the same size, i.e the image should be squared"

@jbesomi jbesomi added good first issue Good for newcomers documentation Improvements or additions to documentation labels Jul 17, 2020
@cedricconol
Copy link
Contributor

Hi @jbesomi @vidyap-xgboost, i'll take this issue.

I also like to add an example in the docstring, but i am not sure how to do this with visualizations. Do you have a suggestion how to do this?

@jbesomi
Copy link
Owner

jbesomi commented Jul 18, 2020

Thank you Sir: assigned!

We cannot really add a visual example in the docstring, but it's cool you asked.

A very interesting solution that soon or later we might want to implement is to create some visual example with sphinx-gallery. Have a look at the documentation of Sphinx-gallery and see if it make sense. For instance, scikit-learn example is using it for the Examples.

We might want to implement that. If you want, I can help you with such task, and apparently also @vidyap-xgboost was interested in contributing :)

@cedricconol
Copy link
Contributor

Based on the discussion in #78, it seems like width and height does not have to be the same so long as both of them are at least 400. The first example in #78 has the same width and height (200, 200) but it is still blurry.

I think we should rephrase it to:
"To reduce blur in the images, width and height should be at least 400."

What do you think @vidyap-xgboost @jbesomi? 😃

@cedricconol
Copy link
Contributor

Thank you Sir: assigned!

We cannot really add a visual example in the docstring, but it's cool you asked.

A very interesting solution that soon or later we might want to implement is to create some visual example with sphinx-gallery. Have a look at the documentation of Sphinx-gallery and see if it make sense. For instance, scikit-learn example is using it for the Examples.

We might want to implement that. If you want, I can help you with such task, and apparently also @vidyap-xgboost was interested in contributing :)

Yup I'm interested in implementing it through sphinx-gallery, I want to help 😄

@vidyap-xgboost
Copy link
Contributor Author

vidyap-xgboost commented Jul 18, 2020

Based on the discussion in #78, it seems like width and height does not have to be the same so long as both of them are at least 400. The first example in #78 has the same width and height (200, 200) but it is still blurry.

I think we should rephrase it to:
"To reduce blur in the images, width and height should be at least 400."

What do you think @vidyap-xgboost @jbesomi?

I was going to point this out and a little late here, thanks for bringing it up and I agree to your point! But can you also make sure to test for width=500 and height=400 (you can also test with higher numbers) and see if any blur is being introduced? Follow #96 to see how I tested it on the terminal directly.

Thank you!

@vidyap-xgboost
Copy link
Contributor Author

@jbesomi @cedricconol According to the numpy docstring guide as suggested in CONTRIBUTING.md:

The length of docstring lines should be kept to 75 characters to facilitate reading the docstrings in text terminals.

But wordcloud seems to have more than 75 characters in its docstring(and yes, its really difficult to read in VS code), can you please take care of that @cedricconol ? I thought I would do it myself but I do not want to interfere with your PR 😃 , hence asking you.

@cedricconol
Copy link
Contributor

Based on the discussion in #78, it seems like width and height does not have to be the same so long as both of them are at least 400. The first example in #78 has the same width and height (200, 200) but it is still blurry.
I think we should rephrase it to:
"To reduce blur in the images, width and height should be at least 400."
What do you think @vidyap-xgboost @jbesomi?

I was going to point this out and a little late here, thanks for bringing it up and I agree to your point! But can you also make sure to test for width=500 and height=400 (you can also test with higher numbers) and see if any blur is being introduced? Follow #96 to see how I tested it on the terminal directly.

Thank you!

Hi @vidyap-xgboost, I tested width=500, height=400 and width=1000, height=800 following your code in #96. It seems like the blur is gone 😄

>>> import texthero as hero
>>> import pandas as pd
>>> df = pd.read_csv('https://github.com/jbesomi/texthero/raw/master/dataset/bbcsport.csv')
>>> import matplotlib
>>> hero.visualization.wordcloud(df['text'],width = 500, height= 400,background_color='White',min_font_size=1)
>>> matplotlib.pyplot.show(block=True)
>>> hero.visualization.wordcloud(df['text'],width = 1000, height= 800,background_color='White',min_font_size=1)
>>> matplotlib.pyplot.show(block=True)

Here are the results:
width=500, height=400
500400

width=1000, height=800
1000800

@cedricconol
Copy link
Contributor

@jbesomi @cedricconol According to the numpy docstring guide as suggested in CONTRIBUTING.md:

The length of docstring lines should be kept to 75 characters to facilitate reading the docstrings in text terminals.

But wordcloud seems to have more than 75 characters in its docstring(and yes, its really difficult to read in VS code), can you please take care of that @cedricconol ? I thought I would do it myself but I do not want to interfere with your PR , hence asking you.

Thanks for the suggestion @vidyap-xgboost, this is noted, it seems like black does not format the docstrings.

@vidyap-xgboost
Copy link
Contributor Author

vidyap-xgboost commented Jul 19, 2020

@jbesomi @cedricconol According to the numpy docstring guide as suggested in CONTRIBUTING.md:

The length of docstring lines should be kept to 75 characters to facilitate reading the docstrings in text terminals.

But wordcloud seems to have more than 75 characters in its docstring(and yes, its really difficult to read in VS code), can you please take care of that @cedricconol ? I thought I would do it myself but I do not want to interfere with your PR , hence asking you.

Thanks for the suggestion @vidyap-xgboost, this is noted, it seems like black does not format the docstrings.

Great work @cedricconol ! 👏 👏
Yes, the docstrings are not formatted by black. Any solution here @jbesomi ?

@jbesomi
Copy link
Owner

jbesomi commented Jul 19, 2020

Hey @vidyap-xgboost @cedricconol! You are doing great, thank you!

Apparently black does not format docstring. I guess we will need to do it ourselves (@vidyap-xgboost we might explain this better in the CONTRIBUTING.md, what do you think?). If you want to know more: Black should have an opinion about doc strings (still open issue)

@vidyap-xgboost
Copy link
Contributor Author

Hey @vidyap-xgboost @cedricconol! You are doing great, thank you!

Apparently black does not format docstring. I guess we will need to do it ourselves (@vidyap-xgboost we might explain this better in the CONTRIBUTING.md, what do you think?). If you want to know more: Black should have an opinion about doc strings (still open issue)

I agree. Explaining in CONTRIBUTING.md sounds good. Thanks for sharing the link!
I would like to work on further changes in documentation and tutorials after the next release since @henrifroese is already working on the documentation. I wouldn't want to interfere and duplicate the work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants