-
-
Couldn't load subscription status.
- Fork 2.7k
Dendrogram class #274
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
Dendrogram class #274
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't require our users to have numpy or scipy installed to use this package. For this dendrogram class, it's OK to rely on these packages, you'll just have to make a few changes in this package organization to make this work:
1 - Move these tests into test_optional. That's the folder that we use for all the functionality that uses optional dependencies like numpy, matplotlib, scipy. Here is a file that you can place these tests into, instead: https://github.com/plotly/python-api/blob/master/plotly/tests/test_optional/test_opt_tracefactory.py
2 - Protect imports. More on that below!
We split our tests into two folders: test_core and test_optional. The only difference is that test_optional contains tests that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done with 3781906
Looks great, thanks for submitting! A few other comments besides those in the code:
- We try to follow pep8 pretty closely so that all of our contributions look and read the same. Code editors usually have a pep8 style plug-in (I use sublime and this plug-in: https://github.com/dreadatour/Flake8Lint).
- Thoughts on adding an option, or a separate FigureFactory, to include a heatmap and a second horizontal dendrogram?
plotly/tools.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be good to keep the docstrings similar across the FigureFactory methods. I used this guideline https://github.com/plotly/dev-docs/blob/master/reference/styleguides/python.md#function-docstrings for the others.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chriddyp and @cldougl thanks! will fix/respond on each comment
as for
Thoughts on adding an option, or a separate FigureFactory, to include a heatmap and a second horizontal dendrogram?
You can already create a horizontal dendrogram using this class and adding them to a heatmap is not that difficult using subplots. Although an option of including dendrograms added to the existing heatmap function would come handy.
plotly/tools.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've been adding things like this with a BETA tag in the docstring. This way we don't have to promise backwards compatibility on these new wrappers. We simply use:
"""
Draw a 2d dendrogram tree.
BETA: May change without warning in a backwards-incompatible way.
...
"""
plotly/tools.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🐄 this is redundant, you've already added it with the :param: info beautifully above :)
Thanks for putting all this work in! There are still a couple things that could use some addressing. Mostly, I think adding more tests to lock down the expected functionality would do.
@theengineear @cldougl Let's go for round 3 and perhaps you have some useful tips for improving orientation plots (see image in the comments above)
@OxanaSachenkova sorry for the radio silence! Let me know if you want me to pull down the branch to fix any conflicts! I'm happy to help with those.
@theengineear yes, I think it's better if you take over to fix the last things and resolve the conflicts. probably will be more efficient
Great, I'll do that today and we can get this in!
Closing this in favor of #312. Thanks @OxanaSachenkova !!!
Hi all, please have a look at the dendrogram wrapper and let me know if you want to add anything else at this point.
All feedback is welcome!