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

Closed
wants to merge 17 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@oxananu
Contributor

oxananu commented Aug 4, 2015

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!

Show outdated Hide outdated plotly/tools.py Outdated
Show outdated Hide outdated plotly/tools.py Outdated
Show outdated Hide outdated plotly/tools.py Outdated
Show outdated Hide outdated plotly/tools.py Outdated
Show outdated Hide outdated plotly/tools.py Outdated
Show outdated Hide outdated plotly/tools.py Outdated
Show outdated Hide outdated plotly/tools.py Outdated
Show outdated Hide outdated plotly/tools.py Outdated
Show outdated Hide outdated plotly/tools.py Outdated
Show outdated Hide outdated plotly/tools.py Outdated
Show outdated Hide outdated plotly/tools.py Outdated
@chriddyp

This comment has been minimized.

Show comment
Hide comment
@chriddyp

chriddyp Aug 4, 2015

Member

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?
Member

chriddyp commented Aug 4, 2015

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?
Show outdated Hide outdated plotly/tools.py Outdated
Show outdated Hide outdated plotly/tools.py Outdated
@oxananu

This comment has been minimized.

Show comment
Hide comment
@oxananu

oxananu Aug 5, 2015

Contributor

@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.

Contributor

oxananu commented Aug 5, 2015

@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.

Show outdated Hide outdated plotly/tools.py Outdated
Show outdated Hide outdated plotly/tools.py Outdated
Show outdated Hide outdated plotly/tools.py Outdated
Show outdated Hide outdated plotly/tools.py Outdated
Show outdated Hide outdated plotly/tools.py Outdated
Show outdated Hide outdated plotly/tools.py Outdated
Show outdated Hide outdated plotly/tools.py Outdated
Show outdated Hide outdated plotly/tools.py Outdated
self.data = []
self.leaves = []
self.sign = {self.xaxis: 1, self.yaxis: 1}
self.layout = {self.xaxis: {}, self.yaxis: {}}

This comment has been minimized.

@theengineear

theengineear Aug 24, 2015

Contributor

Hm, maybe I'm unclear on which xaxis/yaxis you're trying to store here. There's the axis reference which is when you say something like 'xaxis': 'x1' and then there's the layout axis object which is when you say something like 'xaxis2': {'domain': [0, 1]}.

So, I think you're making them serve two purposes, using the full string to name the layout object and trying to take the last digit and use it as a reference. I'd suggest you write a bit of logic in there that handles the case when the last character isn't a digit:

try:
    axis_index = int(axis_string[-1])
except ValueError:
    axis_index = ''
else:
    axis_char = axis_string[0]
    axis_reference = '{}{}'.format(axis_char, axis_index)

Again, sorry if I'm misinterpreting. I think just clearing up the test suite will sort this out.

@theengineear

theengineear Aug 24, 2015

Contributor

Hm, maybe I'm unclear on which xaxis/yaxis you're trying to store here. There's the axis reference which is when you say something like 'xaxis': 'x1' and then there's the layout axis object which is when you say something like 'xaxis2': {'domain': [0, 1]}.

So, I think you're making them serve two purposes, using the full string to name the layout object and trying to take the last digit and use it as a reference. I'd suggest you write a bit of logic in there that handles the case when the last character isn't a digit:

try:
    axis_index = int(axis_string[-1])
except ValueError:
    axis_index = ''
else:
    axis_char = axis_string[0]
    axis_reference = '{}{}'.format(axis_char, axis_index)

Again, sorry if I'm misinterpreting. I think just clearing up the test suite will sort this out.

This comment has been minimized.

@oxananu

oxananu Sep 1, 2015

Contributor

@theengineear I am actually a bit confused about this one, but I think I fixed it in this ebff53e commit and cleaned up here 16c74c1

@oxananu

oxananu Sep 1, 2015

Contributor

@theengineear I am actually a bit confused about this one, but I think I fixed it in this ebff53e commit and cleaned up here 16c74c1

if self.orientation in ['right', 'bottom']:
self.sign[self.yaxis] = 1
else:
self.sign[self.yaxis] = -1

This comment has been minimized.

@theengineear

theengineear Aug 24, 2015

Contributor

This orientation logic doesn't seem to match the left, right, bottom, top that your documentation suggests?

@theengineear

theengineear Aug 24, 2015

Contributor

This orientation logic doesn't seem to match the left, right, bottom, top that your documentation suggests?

This comment has been minimized.

@oxananu

oxananu Sep 1, 2015

Contributor

@theengineear I don't really understand what does not match, could you clarify? I am attaching how the dendrograms look with different orientations now also

screen shot 2015-09-01 at 23 17 38

@oxananu

oxananu Sep 1, 2015

Contributor

@theengineear I don't really understand what does not match, could you clarify? I am attaching how the dendrograms look with different orientations now also

screen shot 2015-09-01 at 23 17 38

This comment has been minimized.

@oxananu

oxananu Sep 1, 2015

Contributor

@cldougl @theengineear P.S. any tips how to move the axis labels together with the actual axis when changing orientation?

@oxananu

oxananu Sep 1, 2015

Contributor

@cldougl @theengineear P.S. any tips how to move the axis labels together with the actual axis when changing orientation?

This comment has been minimized.

@theengineear

theengineear Sep 27, 2015

Contributor

Apologies, I think I may have just read over that too fast, looks good. Also, these are really great plots!!

I wouldn't worry about that for now, let's get this merged in! I'll be more present so that we can get this in soon so we don't keep leaving you hanging.

@theengineear

theengineear Sep 27, 2015

Contributor

Apologies, I think I may have just read over that too fast, looks good. Also, these are really great plots!!

I wouldn't worry about that for now, let's get this merged in! I'll be more present so that we can get this in soon so we don't keep leaving you hanging.

if self.orientation in ['top', 'bottom']:
ys = dcoord[i]
else:
ys = icoord[i]

This comment has been minimized.

@theengineear

theengineear Aug 24, 2015

Contributor

Where does the left and right come in here?

@theengineear

theengineear Aug 24, 2015

Contributor

Where does the left and right come in here?

This comment has been minimized.

@oxananu

oxananu Sep 1, 2015

Contributor

@theengineear in the "else" statement

@oxananu

oxananu Sep 1, 2015

Contributor

@theengineear in the "else" statement

Show outdated Hide outdated plotly/tools.py Outdated
Show outdated Hide outdated plotly/tools.py Outdated
Show outdated Hide outdated plotly/tools.py Outdated
Show outdated Hide outdated plotly/tools.py Outdated
Show outdated Hide outdated plotly/tools.py Outdated
Show outdated Hide outdated plotly/tools.py Outdated
Show outdated Hide outdated plotly/tools.py Outdated
Show outdated Hide outdated plotly/tools.py Outdated
Show outdated Hide outdated plotly/tools.py Outdated
@theengineear

This comment has been minimized.

Show comment
Hide comment
@theengineear

theengineear Aug 24, 2015

Contributor

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.

Contributor

theengineear commented Aug 24, 2015

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 theengineear referenced this pull request Aug 24, 2015

Merged

Distplot #275

@oxananu

This comment has been minimized.

Show comment
Hide comment
@oxananu

oxananu Sep 4, 2015

Contributor

@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)

Contributor

oxananu commented Sep 4, 2015

@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)

@theengineear

This comment has been minimized.

Show comment
Hide comment
@theengineear

theengineear Sep 27, 2015

Contributor

I still think this is off a bit, the xs value should be x, right?

Contributor

theengineear commented on plotly/tests/test_optional/test_opt_tracefactory.py in ebff53e Sep 27, 2015

I still think this is off a bit, the xs value should be x, right?

This comment has been minimized.

Show comment
Hide comment
@etpinard

etpinard Sep 27, 2015

Member

yep

Member

etpinard replied Sep 27, 2015

yep

This comment has been minimized.

Show comment
Hide comment
@etpinard

etpinard Sep 27, 2015

Member

the regex:
image

Member

etpinard replied Sep 27, 2015

the regex:
image

@theengineear

This comment has been minimized.

Show comment
Hide comment
@theengineear

theengineear Sep 27, 2015

Contributor

@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.

Contributor

theengineear commented Sep 27, 2015

@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.

@oxananu

This comment has been minimized.

Show comment
Hide comment
@oxananu

oxananu Sep 29, 2015

Contributor

@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

Contributor

oxananu commented Sep 29, 2015

@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

@theengineear

This comment has been minimized.

Show comment
Hide comment
@theengineear

theengineear Sep 30, 2015

Contributor

Great, I'll do that today and we can get this in!

Contributor

theengineear commented Sep 30, 2015

Great, I'll do that today and we can get this in!

@theengineear

This comment has been minimized.

Show comment
Hide comment
@theengineear

theengineear Oct 1, 2015

Contributor

Closing this in favor of #312. Thanks @OxanaSachenkova !!!

Contributor

theengineear commented Oct 1, 2015

Closing this in favor of #312. Thanks @OxanaSachenkova !!!

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