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

HeroTypes in Representation; DataFrame in _types #157

Merged
merged 27 commits into from
Sep 21, 2020
Merged

HeroTypes in Representation; DataFrame in _types #157

merged 27 commits into from
Sep 21, 2020

Conversation

henrifroese
Copy link
Collaborator

@henrifroese henrifroese commented Aug 22, 2020 •
edited
Loading

  • switch DocumentTermDF in for RepresentationSeries in _types.py
  • add functionality for decorator @InputSeries to handle several allowed input types
  • Add typing decorator/hints to representation.py
  • add tests for DocumentTermDF type in test_types.py

NOTE: only so many commits/lines as this builds on #156

mk2510 and others added 11 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 henrifroese added the enhancement New feature or request label Aug 23, 2020
This was referenced Aug 23, 2020
Open
Open
Open
Open
@mk2510 mk2510 changed the title HeroTypes in Representation; DocumentTermDF in _types HeroTypes in Representation; DataFrame in _types Sep 4, 2020
@mk2510
Copy link
Collaborator

mk2510 commented Sep 4, 2020

we have now also adopted the type checks. The given DataFrame should not be a multicolumn one, but a "normal" Sparse DataFrame. We decided to not call it SparseDataFrame, as in the future the DataFrame may not have to be sparse. :octocat:

@mk2510 mk2510 mentioned this pull request Sep 5, 2020
Merged
@jbesomi
Copy link
Owner

jbesomi commented Sep 8, 2020

Nice!

Just realizing that this PR solve some of the issues of #180

I will review the code details once #156 is merged.

Overall questions: why do we need the DocumentTermDF type? That's just a DataFrame with numeric-only values, right? As discussed, I would not have a specific type for this, at least for now.

@jbesomi jbesomi marked this pull request as draft September 8, 2020 11:38
@ryangawei ryangawei mentioned this pull request Sep 8, 2020
Open
@mk2510
Copy link
Collaborator

mk2510 commented Sep 9, 2020

Overall questions: why do we need the DocumentTermDF type? That's just a DataFrame with numeric-only values, right? As discussed, I would not have a specific type for this, at least for now.

We changed it to only DataFrame, as it also could represent a different constellation than DocumentTermDF. But when we remove this type from our pandas types, we have an Incomplete TypeCheck. For now the DataFrame typcheck is not as much worth as the Series Types, but it completes the typing of every input.

@henrifroese
Copy link
Collaborator Author

henrifroese commented Sep 10, 2020 •
edited
Loading

We could of course just use the actual pd.DataFrame type in our type checks. Arguments:

  • Pro: easier for us; not confusing to users (as they might wonder "what's special about this DataFrame in texthero?")
  • Con: our documentation will then not link to a hero series article with a nice docstring explaining that this is to represent a matrix etc (as the _types module is not only useful for typechecks but also for documentation).

@henrifroese henrifroese mentioned this pull request Sep 12, 2020
Closed
@henrifroese @mk2510
Remove check for nlevels
85076b8 
Co-authored-by: Maximilian Krahn <maximilian.krahn@icloud.com>
@henrifroese
Copy link
Collaborator Author

We have now re-merged #156 in this and solved the last remaining issues we saw. Ready to review/merge now from our side 🥉

@henrifroese henrifroese marked this pull request as ready for review September 12, 2020 12:14
jbesomi
jbesomi reviewed Sep 14, 2020
View reviewed changes
tests/test_representation.py Outdated Show resolved Hide resolved
texthero/_types.py Outdated Show resolved Hide resolved
texthero/_types.py Outdated Show resolved Hide resolved
texthero/_types.py Show resolved Hide resolved
@jbesomi jbesomi mentioned this pull request Sep 14, 2020
Open
@henrifroese @mk2510
incorporate suggested changes
91ed11a 
Co-authored-by: Maximilian Krahn <maximilian.krahn@icloud.com>
@jbesomi jbesomi marked this pull request as draft September 14, 2020 15:43
@henrifroese
Copy link
Collaborator Author

From our side, this is now ready to be re-reviewed / merged 🤞 🍼

@henrifroese henrifroese marked this pull request as ready for review September 14, 2020 16:26
@henrifroese henrifroese requested a review from jbesomi September 18, 2020 16:00
@jbesomi jbesomi merged commit b7827ca into jbesomi:master Sep 21, 2020
@jbesomi
Copy link
Owner

jbesomi commented Sep 21, 2020 •
edited
Loading

Thanks! Merged 🎉 🎉 That's a big milestone!

@henrifroese
Copy link
Collaborator Author

Great! 🎉🎉🎉. We will now go through all PRs in the merge checklist that are branched from this and change them accordingly! 💡👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ryangawei ryangawei ryangawei left review comments

@jbesomi jbesomi Awaiting requested review from jbesomi

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.
4 participants
@henrifroese @mk2510 @jbesomi @ryangawei