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

Visualization #212

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

anderspitman
Copy link
Contributor

No description provided.

Invokes Rscript pedigree_and_layout.R to compute the layout
(depends on R kinship2 package), and bundles the output into
an html template. The final output is an html file which
contains all the visualization data except for some 3rd party
libraries like JQuery, d3, and Bootstrap, which are currently
being pulled from the web by the user's browser at runtime.

Note: this functionality depends on pedigree_and_layout.R and
the index.html template, which are not included in this commit,
since they are part of the dng-vis repo. I'll probably add them
in a separate commit.
Also checks that kinship2 and jsonlite R packages are installed
and issues a warning if not.
Use find_package_handle_standard_args to get standard messages.
@dng-jenkins
Copy link

Can one of the admins verify this patch?

@anderspitman
Copy link
Contributor Author

I'm including pedigree_and_layout.R and index.html, even though those are part of the dng-vis repo and will change along with that. I'm not sure how we want to handle that dependency, so I just did it like treecall where we manually copy over a version for now.

@reedacartwright
Copy link
Contributor

reedacartwright commented Dec 23, 2016 via email

@codecov-io
Copy link

codecov-io commented Dec 23, 2016

Current coverage is 58.64% (diff: 100%)

Merging #212 into develop will increase coverage by 0.99%

@@            develop       #212   diff @@
==========================================
  Files            88         88          
  Lines          8382       7989   -393   
  Methods         520        520          
  Messages          0          0          
  Branches       4839       4779    -60   
==========================================
- Hits           4832       4685   -147   
+ Misses         1777       1685    -92   
+ Partials       1773       1619   -154   

Powered by Codecov. Last update c4d067f...c5cd796

@anderspitman
Copy link
Contributor Author

How is it possible for coverage to have changed that much?

Made the development and deployment pipelines much closer. The
same scripts are now used for both for the most part.

Also added argument parsing to dng mutmap
@reedacartwright
Copy link
Contributor

If mutmap is going to be an "official" part of DNG, then it will need to be developed as part of this repository.

Also, why do we have a shell script that calls a Python script that calls an R script? Can't we just have an executable R script that does everything? Or am I missing something?

@anderspitman
Copy link
Contributor Author

Cool, I'll move all the visualization code to be under src/mutmap and carry on development from there.

The multiple scripts are mostly to keep things modular. I don't want our layout to be tied to R, so I didn't write the other logic in that. I started in bash but switched to Python once I started added argument parsing. I originally had dng-mutmap simply be the Python script, but changed it so that I could use the same python script during development. That saves me time and also accomplishes a little dogfooding. So the shell script is mostly acting as an intermediate between CMAKE and python. That way there isn't any @@ replacement happening in the python or R scripts, which keeps them more generally usable.

@reedacartwright
Copy link
Contributor

Right now, you are not doing anything with the shell and python script that you cannot do in R. Before we go to a three-language solution, I would like to see an attempt to try to do it with one script.

@anderspitman
Copy link
Contributor Author

I guess my biggest argument at this point is that it's already finished and working, and would be the same amount of work to change it later as it would be now. In general, anything that needs to be done in R will necessitate me first learning how to do that thing in R (argument parsing is a great example), whereas I'm already familiar with python and can add features quickly. I'd love to learn some more R but I don't think it's a good tool for this.

I think the R dependency is the one we should be the most wary about. Eventually I would like to replace the layout calculation with C++ or javascript. Python is a much more common utility language, so other collaborators would expect to see it used for the sorts of things I'm doing here. If we do that logic in R, it will raise the bar for external contributions.

@anderspitman
Copy link
Contributor Author

Also getting rid of the shell shim would mean complicating the development pipeline.

@anderspitman
Copy link
Contributor Author

But I don't mind doing it if you sure that's the way you want to go.

@anderspitman
Copy link
Contributor Author

Another use case I just thought of. An entire build takes about 3 seconds currently. Most of that time is spent warming up R and loading the libraries. I was just working on a way to have python keep a warm Rscript instance running that you can just feed into it's stdin and get results back. This should cut the compile loop down to basically instantaneous, which would help development. This is all pretty straightforward in python, but I'm not sure if it's even possible in R. A quick search doesn't look too hopeful.

@anderspitman
Copy link
Contributor Author

I think this should be superceded by #213

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