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.

Sign up for GitHub

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

Jump to bottom

Visualize hero.describe with plots #178

Open
wants to merge 25 commits into
base: master
Choose a base branch
from
Open

Visualize hero.describe with plots #178

wants to merge 25 commits into from

Conversation

mk2510
Copy link
Collaborator

@mk2510 mk2510 commented Sep 6, 2020 •
edited by henrifroese
Loading

This new function in the visualization module allows users to easily visualize the describe function. We belive, that this will be often use, to display the key characteristics of the DataFrame. 🥉

This function simply calls the describe functions and then with plotly those plots will be generated and displayed in a new browser window. As it is a lot of code to display them, we belive, that it will be quite an enrichment to the library 🏑

Note: travis/setup changed because of #171 . This is branched straight from PR #168 which is why it has so many lines changed 🥡 .

This is a short colab demo of the new function 🤖

Max stinkt

mk2510 and others added 22 commits August 18, 2020 22:06
@mk2510 @henrifroese
added MultiIndex DF support
fa342a9 
suport MultiIndex as function parameter

returns MultiIndex, where Representation was returned

* missing: correct test


Co-authored-by: Henri Froese <hf2000510@gmail.com>
@henrifroese
beginning with tests
59a9f8c
@mk2510 @henrifroese
implemented correct sparse support
19c52de 
*missing: test adopting for new types


Co-authored-by: Henri Froese <hf2000510@gmail.com>
@mk2510
Merge branch 'master_upstream' into change_representation_to_multicolumn
66e566c
@mk2510
added back list() and rm .tolist()
41f55a8
@mk2510
rm .tolist() and added list()
217611a
@mk2510
Adopted the test to the new dataframes
6a3b56d
@mk2510
wrong format
b8ff561
@henrifroese
Address most review comments.
e3af2f9
@henrifroese
Add more unittests for representation
77ad80e
@henrifroese @mk2510
- Update _types.py with DocumentTermDF
f7eb7c3 
- add functionality for decorator @InputSeries to handle several allowed input types
- Add typing decorator/hints to representation.py
- add tests for _types DocumentTermDF

Co-authored-by: Maximilian Krahn <maximilian.krahn@icloud.com>
@henrifroese
Fix DocumentTermDF example DataFrame column names
4937a4f
@henrifroese @mk2510
Implement hero.describe
5fc720c 
Co-authored-by: Maximilian Krahm <maximilian.krahn@icloud.com>
@henrifroese
Change hero.describe to return DataFrame for pretty-printing in Noteb…
55dcd7f 
…ooks
@mk2510
Auto stash before merge of "hero_describe_function" and "origin/hero_…
f3bbc08 
…describe_function"
@mk2510 @henrifroese
Add tests for hero.describe
9e72c85 
Co-authored-by: Henri Froese <henri.froese@yahoo.com>
@mk2510
added right black version
5aaa579
@mk2510
added test and formatting
4d398a0
@mk2510
added correct order
aa3aa56
@mk2510 @henrifroese
added test and formatting:
ea5c640 
Co-authored-by: Henri Froese <hf2000510@gmail.com>
@mk2510
added correct order
d72128f
@mk2510
Merge remote-tracking branch 'origin/visualize_describe_with_plots' i…
f6b2fbf 
…nto visualize_describe_with_plots
@mk2510 mk2510 added the enhancement New feature or request label Sep 6, 2020
@henrifroese henrifroese changed the title Visualize describe with plots Visualize hero.describe with plots Sep 6, 2020
@jbesomi
Copy link
Owner

jbesomi commented Sep 7, 2020

That's beautiful! 😍

  • What does it happen if you run visualize_describe on an empty DF? (we should test that behavior for all heroes functions ...)
  • (opinionable comment) What if we have all graphs in a single column? That's might be better for visualization on narrow displays

jbesomi
jbesomi reviewed Sep 7, 2020
View reviewed changes
texthero/visualization.py Outdated Show resolved Hide resolved
@henrifroese
Copy link
Collaborator

What does it happen if you run visualize_describe on an empty DF? (we should test that behavior for all heroes functions ...)

Currently we get an error but that's just because of the default arguments (see below). Agree we should test that behavior for all hero functions, probably in a separate PR in the future.

(opinionable comment) What if we have all graphs in a single column? That's might be better for visualization on narrow displays

We tried it out and think it looks good as-is on 13 inch screens (which is the standard "small" laptop size).

From your review

labels_col_name="topic" not sure it make sense to have as default value "topic". Moreover, there might be multiple non-text columns. ...

Yes, the default arguments really make no sense, we just used them for testing purposes and forgot to remove them 🤦 🤦‍♂️ 🤦‍♀️

Additionally, not sure what you mean / how we would integrate several text columns? We thought that there are two main scenarios:

  1. User has text in a column -> he uses our function without the labels stuff, everything's great

  2. User has text in a column and labels in a column (e.g. to train a classifier) -> he uses our function with the labels stuff, everything's great

Additionally just noticed that there isn't a docstring (totally forgot this). Will convert this to draft and address all concerns + add docstring. Will update here and post screenshots when it's done.

@henrifroese
Incorporate suggested changes.
4cb1058 
- Docstring
- Better signature
@henrifroese
Copy link
Collaborator

henrifroese commented Sep 9, 2020 •
edited
Loading

Now changed the function signature to be in line with hero.describe (which really makes sense as this is a visualization of hero.describe). Additionally, default arguments are changed.

We noticed that this function cannot handle empty input because of hero.tokenize cannot handle empty input. This is probably a low-priority bug as of course empty input does not make any sense whatsoever.

Note: the merge conflicts should be resolved when we're farther along the merge checklist; we'll revisit this

@henrifroese henrifroese mentioned this pull request Sep 12, 2020
Open
17 tasks
@jbesomi jbesomi marked this pull request as draft September 14, 2020 15:59
@mk2510
Copy link
Collaborator Author

mk2510 commented Sep 22, 2020

we just went through this PR again and it is ready to be reviewed / to be merged 🔢 🧶

@mk2510 mk2510 marked this pull request as ready for review September 22, 2020 20:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@henrifroese henrifroese henrifroese left review comments

@jbesomi jbesomi jbesomi left review comments

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mk2510 @jbesomi @henrifroese