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

Fixes #317 #323

Merged
merged 4 commits into from
Aug 12, 2020
Merged

Fixes #317 #323

merged 4 commits into from
Aug 12, 2020

Conversation

wasade
Copy link
Member

@wasade wasade commented Aug 11, 2020 •
edited
Loading

Fixes #317 and uses biom.Table rather than pd.DataFrame for the feature table

@fedarko fedarko self-requested a review August 11, 2020 22:15
fedarko
fedarko requested changes Aug 11, 2020
View reviewed changes
Copy link
Collaborator

@fedarko fedarko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much @wasade! This looks good to me, a few small suggestions but nothing huge. Looking forward to being able to run Empress on EMP-scale datasets without exploding laptops 🌴

empress/compression_utils.py Outdated Show resolved Hide resolved
empress/compression_utils.py Outdated Show resolved Hide resolved
empress/compression_utils.py Outdated Show resolved Hide resolved
empress/compression_utils.py Outdated Show resolved Hide resolved
empress/core.py Show resolved Hide resolved
empress/tools.py Outdated Show resolved Hide resolved
empress/tools.py Show resolved Hide resolved
tests/python/test_compression_utils.py Outdated Show resolved Hide resolved
tests/python/test_core.py Show resolved Hide resolved
@fedarko
Copy link
Collaborator

fedarko commented Aug 11, 2020 •
edited
Loading

It looks like the tests are failing because a few "test tables" are still being created as DataFrames:

  • self.table in tests/python/test_tools.py
  • table in tests/python/make-dev-page.py (looks like this can be fixed in ~one line, by replacing line 67 with table = table.view(biom.Table) (and replacing the pandas import with a biom import)

@wasade
Copy link
Member Author

wasade commented Aug 11, 2020

Thanks, @fedarko! I thought I had issued a full pytest call before the PR but had only tested out two of the modules. More shortly.

@emperor-helper
Copy link

The following artifacts were built for this PR: empire-biplot.qzv, empire.qzv, empress-tree.qzv

@fedarko fedarko self-requested a review August 12, 2020 00:44
fedarko
fedarko requested changes Aug 12, 2020
View reviewed changes
Copy link
Collaborator

@fedarko fedarko left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

two extremely minor comments (and I think https://github.com/biocore/empress/pull/323/files#r468897589 is unresolved), but after these i think this is good to merge.

tests/python/test_compression_utils.py Outdated Show resolved Hide resolved
@fedarko
Copy link
Collaborator

fedarko commented Aug 12, 2020

Ok, everything looks good! Thanks so much @wasade! (╯°□°)╯︵ ┻━┻

@fedarko fedarko merged commit 6b73e58 into biocore:master Aug 12, 2020
@wasade
Copy link
Member Author

wasade commented Aug 12, 2020

woooo!!! thanks!!!

@ElDeveloper
Copy link
Member

Thanks so much @wasade!

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

@fedarko fedarko fedarko requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use the biom.Table throughout the codebase
4 participants
@wasade @fedarko @emperor-helper @ElDeveloper