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

pileup.js component #543

Merged
merged 24 commits into from
Apr 1, 2021
Merged

pileup.js component #543

merged 24 commits into from
Apr 1, 2021

Conversation

akmorrow13
Copy link
Contributor

@akmorrow13 akmorrow13 commented Feb 7, 2021

About

  • I am closing an issue
  • This is a new component
  • I am adding a feature to an existing component, or improving an existing feature

Description of changes

Adding a component that uses pileup.js to visualize genomic datasets.

Current TODOs/issues:

  • I am unable to test out the demos, as I cannot access the data folder through the app. How do you add data to the app so you can access it in the demos?
  • Still need to create a demo for RNA-seq visualization.
  • Issue with visualization of mouse genes for RNA-seq demo
  • Issue with low coverage for RNA-seq demo
  • dash needs modification of ReactDOM components in pileup.js to work properly

@akmorrow13 akmorrow13 changed the title Pileup pileup.js component Feb 7, 2021
@alexcjohnson
Copy link
Collaborator

@akmorrow13 this is looking great! Re: your question:

I am unable to test out the demos, as I cannot access the data folder through the app. How do you add data to the app so you can access it in the demos?

Most of the demo apps include data in their own subdirectory - in fact it's not clear to me if there's still any purpose to the root data dir...

Here for example is what the alignment chart demo does:

DATAPATH = os.path.join(os.path.dirname(os.path.abspath(__file__)), 'data')
# Datasets
with open(os.path.join(DATAPATH, 'sample.fasta'), encoding='utf-8') as data_file:
dataset1 = data_file.read()

@HammadTheOne I don't get how these demos are supposed to be used locally... when I've tried that I've had to modify the demo code as it can't find layout_helper am I missing something? Here for example is what I did while reviewing NGL:

 import dash_bio

-try:
-    from layout_helper import run_standalone_app
-except ModuleNotFoundError:
-    from .layout_helper import run_standalone_app
+# try:
+#     from layout_helper import run_standalone_app
+# except ModuleNotFoundError:
+#     from .layout_helper import run_standalone_app

 # Preset colors for the shown molecules
 COLORS = [
@@ -861,9 +861,13 @@ def callbacks(_app):


 # only declare app/server if the file is being run directly
-if 'DEMO_STANDALONE' not in os.environ:
-    app = run_standalone_app(layout, callbacks, header_colors, __file__)
-    server = app.server
+# if 'DEMO_STANDALONE' not in os.environ:
+#     app = run_standalone_app(layout, callbacks, header_colors, __file__)
+#     server = app.server

 if __name__ == '__main__':
+    import dash
+    app = dash.Dash()
+    app.layout = layout
+    callbacks(app)
     app.run_server(debug=True, port=8050)

@akmorrow13 regardless, feel free to just create a normal standalone app for the demo and we can tweak it.

@akmorrow13
Copy link
Contributor Author

Thanks @alexcjohnson for the response. The difference between pileup and the alignment-chart app is that alignment-chart reads in data files external to the app. However, pileup.js needs genomic files served through the app so it can make http calls. However, when the app is started, the data directory is not included in the app. In fact, only css files in the assets directory is included. I am sure there is a way to modify this, I just haven't found it in the documentation yet.

@alexcjohnson
Copy link
Collaborator

Ah ok - then put these files in an assets directory next to the app instead of data. That way they will be automatically made available to requests at the /assets/<filename> URL. JS and CSS files in this directory get automatically included on the page, but other extensions do not. Does that work, to point the component to /assets/... URLs for data?

@akmorrow13
Copy link
Contributor Author

Thanks @alexcjohnson. Any non-css files included in assets are not added once the application starts. I am wondering if there is a file filter somewhere that determines which files can be added to assets.

@akmorrow13
Copy link
Contributor Author

@alexcjohnson this actually works, they are just not visible in the inspector. Thanks!

@akmorrow13
Copy link
Contributor Author

Yes, sorry for the confusion @alexcjohnson . Everything is working great now.

@akmorrow13
Copy link
Contributor Author

@HammadTheOne This is ready for review. I can clean up the commit history if needed when complete. @jackparmer mentioned we need documentation for the components. Where should the documentation go and what is the preferred structure for these docs?

@HammadTheOne
Copy link
Contributor

@akmorrow13 Thanks for the update! Just from glancing over it it looks great, but I'll do a thorough review shortly.

Documentation for this component would go in the dash_bio chapter of the dash-docs repo which can be found here.

For structuring the chapter, it might be helpful to take a look at a commit I made for adding the IGV component docs recently which follows the same basic structure as most of the other components in the library. The main things we want to add are an entry to content_module.py and index.py respectively, and a default example, all within the dash-bio chapter.

@HammadTheOne HammadTheOne marked this pull request as ready for review March 16, 2021 00:58

const RealPileup = lazy(LazyLoader.pileup);
/**
* The Pileup component is an genome visualization component
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* The Pileup component is an genome visualization component
* The Pileup component is a genome visualization component

Object defining genomic location.
Of the format: {contig: 'chr17', start: 7512384, stop: 7512544}
*/
range: PropTypes.shape({
Copy link
Contributor

Choose a reason for hiding this comment

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

We'll want to use exact over shape here, unless there's any additional arbitrary keys supported by this prop. That way, we'll have explicit warnings if any extra properties are passed.

Suggested change
range: PropTypes.shape({
range: PropTypes.exact({

* Name of visualization.
(ie coverage, genome, genes, etc.)
*/
viz: PropTypes.oneOf(PILEUP_VIZ_TYPES),
Copy link
Contributor

Choose a reason for hiding this comment

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

Just want to note here that we may want to explicitly list the PILEUP_VIZ_TYPES and PILEUP_SOURCE_TYPES in our docs once we have a chapter for this component, so users wouldn't have to dig through the source code or external docs to find out which ones are supported.

@akmorrow13
Copy link
Contributor Author

Thanks for the feedback @HammadTheOne , these have all been resolved.

@HammadTheOne
Copy link
Contributor

Thanks @akmorrow, the changes look great and it's nice that you exposed the vizOptions key; I can see that being useful to set with Dash callbacks.

There's just a few very minor suggestions I have, but otherwise it looks good to me 💃

@alexcjohnson I think this PR is ready for a final pass from you, in case there's anything I may have missed.

@akmorrow13
Copy link
Contributor Author

Thank you @HammadTheOne for the comments, the suggestions have been applied. Will this need to be merged in before I can work on the docs locally?

@HammadTheOne
Copy link
Contributor

Thank you @HammadTheOne for the comments, the suggestions have been applied. Will this need to be merged in before I can work on the docs locally?

Thanks @akmorrow13! You should be able to work on the docs locally and even open a PR for them prior to merging this PR, the only caveat would be re-installing this branch of the dash-bio package after installing the requirements.txt of the dash-docs since it would install an older release. If you run into any problems with that, please let me know.

Once this PR is merged we'll most likely make a 0.7.0 release which will include this component and we can update the docs requirements and merge the new chapter in after that's done.

@akmorrow13
Copy link
Contributor Author

Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

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

@akmorrow13 this looks great! 💃 I just had two minor suggestions, and I'm trying to figure out why the CI tests aren't being triggered - @HammadTheOne I mentioned this to @rpkyle since he was working on this earlier re: circleci contexts, it would really be nice to figure this out and see the tests passing before merging.

@HammadTheOne HammadTheOne merged commit 9e5da85 into plotly:master Apr 1, 2021
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