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.

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

Clustergram: app improvements #70

Merged
merged 15 commits into from
Dec 6, 2018
Merged

Conversation

shammamah-zz
Copy link
Contributor

@shammamah-zz shammamah-zz commented Dec 3, 2018

Addresses #64.

@mtwichan mtwichan temporarily deployed to dash-bio-test-pr-70 December 3, 2018 22:03 Inactive
@shammamah-zz shammamah-zz changed the title Restyle app; clustergram now on right hand side and larger, and color… Clustergram: app improvements Dec 3, 2018
@mtwichan mtwichan temporarily deployed to dash-bio-test-pr-70 December 4, 2018 17:17 Inactive
@mtwichan mtwichan temporarily deployed to dash-bio-test-pr-70 December 4, 2018 18:34 Inactive
@mtwichan mtwichan temporarily deployed to dash-bio-test-pr-70 December 4, 2018 20:05 Inactive
@mtwichan mtwichan temporarily deployed to dash-bio-test-pr-70 December 4, 2018 20:06 Inactive
@mtwichan mtwichan temporarily deployed to dash-bio-test-pr-70 December 4, 2018 21:08 Inactive
@mtwichan mtwichan temporarily deployed to dash-bio-test-pr-70 December 5, 2018 03:11 Inactive
@mtwichan mtwichan temporarily deployed to dash-bio-test-pr-70 December 5, 2018 03:51 Inactive
@mtwichan mtwichan temporarily deployed to dash-bio-test-pr-70 December 5, 2018 04:29 Inactive
@shammamah-zz shammamah-zz mentioned this pull request Dec 5, 2018
@mtwichan mtwichan temporarily deployed to dash-bio-test-pr-70 December 5, 2018 22:20 Inactive
@mtwichan mtwichan temporarily deployed to dash-bio-test-pr-70 December 5, 2018 22:25 Inactive
@mtwichan mtwichan temporarily deployed to dash-bio-test-pr-70 December 5, 2018 23:24 Inactive
@mtwichan mtwichan temporarily deployed to dash-bio-test-pr-70 December 5, 2018 23:58 Inactive
@mtwichan mtwichan temporarily deployed to dash-bio-test-pr-70 December 6, 2018 06:45 Inactive
Copy link
Contributor

@Bachibouzouk Bachibouzouk left a comment

Choose a reason for hiding this comment

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

  • I like the color scheme !
  • you could maybe place the "Columns to display" and "Rows to display" at the end of the option list as their size can vary significantly, so the layout of the other options stays relatively the same :)
  • you could also add a title prop for the "Add or remove all group markers:" div to explain how to use it and what it does :)

I'll approve the PR so you can have the latest version ready for the demo and because the changes I suggest are only improvements :)

# they must be converted back
if(isinstance(xs, list)):
xs = [x if x is not None else np.nan for x in xs]
xs = np.array(xs)
Copy link
Contributor

Choose a reason for hiding this comment

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

In terms of speed looping explicitely over arrays in python is slover than using pandas or numpy functions, it would only be important for superlarge datasets though

xs = t['row'][i]['x']
ys = t['row'][i]['y']

if(isinstance(xs, list)):
Copy link
Contributor

Choose a reason for hiding this comment

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

almost the same code as above but with 'row' instead of 'col', could be done in a function for easier maintenance

@shammamah-zz shammamah-zz merged commit 85c6487 into master Dec 6, 2018
@shammamah-zz shammamah-zz deleted the clustergram-review-fix branch December 6, 2018 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants