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

app_needleplot loading data improvements #51

Merged
merged 26 commits into from
Dec 4, 2018
Merged

Conversation

Bachibouzouk
Copy link
Contributor

@Bachibouzouk Bachibouzouk commented Nov 28, 2018

Dash Bio pull request

  • This is a new component
  • I am adding a feature to an existing component

Name and description of your component

-added a dcc.Store to share data between the callbacks
-added the option to load protein domains independently from the mutation data, the user can now download the protein domain from UniProt/PFAM database and load their own mutation data on top of it (the protein domains can also be loaded from an individual file as well)
-documented the callbacks and the code
-deleted sample data files which weren't used anymore for needleplot app

addresses #56 and #64

Before asking for a review

PR merging checklist

Please make sure you have done these things before asking for approval to finally merge.

  • Address all comments in the code review.
  • Make sure you are up-to-date with the latest master: checkout the master branch with git checkout master, pull from latest master git pull, checkout to your branch git checkout your-branch, merge master into your branch git merge master. Resolve conflicts.
  • Run your application successfully in a virtual environment and test that other apps still run
  • Ask for a final review and look for a 💃!
  • Have your dancer? Merge to master!

DDS Deployment

  • Get the developers user credentials from a dash-bio team member.
  • Checkout the master branch of the dash-bio repository, make sure you have the latest master (git pull).
  • Check remotes associated to your repository: git remote -v, make sure you have the dash-bio one: https://dash-gallery.plotly.host/GIT/dash-bio.
  • If you don't have the dash-bio remote add it with https, ssh will not work: git remote add dash-bio https://dash-gallery.plotly.host/GIT/dash-bio.
  • You will be asked to enter the developers user credentials, have them ready.
  • Push local master to the dds dash-bio master: git push dash-bio master.

DDS Deployment Debugging

  • If running git push dash-bio master you may encounter the following issue:
remote: User <username> does not have permissions to run git-hook on dash-bio, or dash-bio does not exist
remote: Access denied
To https://dash-gallery.plotly.host/GIT/dash-bio
 ! [remote rejected] master -> master (pre-receive hook declined)
error: failed to push some refs to 'https://dash-gallery.plotly.host/GIT/dash-bio'

This may be because the git credentials are saved on the client-side. In order to solve this, do the following:

Windows

Go to: Control Panel -> User Accounts -> Manage your credentials -> Windows Credentials -> Under Generic Credentials find: git:https://dash-gallery.plotly.host -> Remove credential OR edit username/password to provided account.

OS X

Go to: Keychain Access -> Search for dash-gallery.plotly.host -> Change "Account" to "developers" -> Click on "show password" -> Change the password to the one provided -> Click "Save Changes". 

@mtwichan mtwichan temporarily deployed to dash-bio-test-pr-51 November 28, 2018 17:10 Inactive
Copy link
Contributor

@shammamah-zz shammamah-zz left a comment

Choose a reason for hiding this comment

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

Really cool! It would be nice to do the same with other database APIs for some of the other components.

tests/dash/app_needleplot.py Outdated Show resolved Hide resolved
tests/dash/app_needleplot.py Outdated Show resolved Hide resolved
tests/dash/app_needleplot.py Outdated Show resolved Hide resolved
tests/dash/utils/needle_plot_parser.py Outdated Show resolved Hide resolved
tests/dash/utils/needle_plot_parser.py Outdated Show resolved Hide resolved
@Bachibouzouk Bachibouzouk mentioned this pull request Dec 1, 2018
20 tasks
Copy link
Contributor

@shammamah-zz shammamah-zz left a comment

Choose a reason for hiding this comment

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

Some minor suggestions. Other than that it looks good!

import dash_core_components as dcc
import dash_html_components as html
from dash.dependencies import Input, Output, State
import dash_bio

from .utils.needle_plot_parser import UniprotQueryBuilder, extract_mutations, EMPTY_MUT_DATA,\
Copy link
Contributor

Choose a reason for hiding this comment

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

I was wondering why this is in utils for tests/dash? It might belong in dash_bio/utils, and that way other apps can use it too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it starts to become big enough for that, some of it is specific to needleplot data parser though, should I still separate the functions that can be used by other apps while moving the sharable code to dash_bio/utils?

Copy link
Contributor

Choose a reason for hiding this comment

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

@Bachibouzouk I just reviewed your current solution and it looks great :)

tests/dash/app_needle_plot.py Outdated Show resolved Hide resolved
else:
return ''

div = []
Copy link
Contributor

Choose a reason for hiding this comment

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

You might not have to initialize this; at the end you can put return [] instead of return div and then on line 476 have

return html.Div(
    [....]
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion :)

@@ -430,15 +514,36 @@ def toggle_individual_domain_loading_in_upload(domains_opt, div_style):
@app.callback(
Output('needle-domain-query-info-div', 'style'),
[Input('needle-protein-domains-select-checklist', 'values')],
[State('needle-domain-query-info-div', 'value')]
[
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe put these on the same line?

    [State('needle-domain-query-info-div', 'value'),
     State('needle-dataset-select-radio', 'value')]

@mtwichan mtwichan temporarily deployed to dash-bio-test-pr-51 December 3, 2018 19:34 Inactive
@mtwichan mtwichan temporarily deployed to dash-bio-test-pr-51 December 3, 2018 21:00 Inactive
Copy link
Contributor

@shammamah-zz shammamah-zz left a comment

Choose a reason for hiding this comment

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

Some minor changes

@@ -1,63 +1,15 @@
import base64
Copy link
Contributor

Choose a reason for hiding this comment

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

Cool! It'll be nice to have all of these tools together for other people to use as well :)

@@ -88,6 +89,14 @@ def description():
Also known under the lollipop plot name.'


def header_colors():
return {
'bg_color': '#00cc96',
Copy link
Contributor

Choose a reason for hiding this comment

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

The logo is a bit difficult to see on this color, could you maybe choose something with more contrast?

Copy link
Contributor

Choose a reason for hiding this comment

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

default header looks good : ) maybe we could leave it like that as it seems to look good on all apps

Copy link
Contributor

Choose a reason for hiding this comment

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

@VeraZab I thought it would be nice to use different colours for all of our headers, since right now my Clustergram app is the only one with a different colour.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess that's not on dds, I don't see it, thought Pierre's was the only one using a different one. I'll let you guys decide.

html.Div(
id='needle-dataset-select-div',
title='"Demo dataset" choice will allow you to play with the options.\n'
'"UniProt dataset" choice will retrieve protein domain '
Copy link
Contributor

Choose a reason for hiding this comment

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

Use \ to break multiline strings

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I use \ it leaves a bunch of trailing whitespaces in the hover text, so I prefer not to

),
html.Div(
id='needle-protein-domains-select-div',
title='If checked, it will allow the user to load mutation data such '
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above

@shammamah-zz
Copy link
Contributor

Everything looks great :) 💃

@Bachibouzouk Bachibouzouk merged commit 034dc55 into master Dec 4, 2018
@Bachibouzouk Bachibouzouk deleted the needle_plot_store_app branch February 1, 2019 09:28
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.

4 participants