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

Potpourri of graph viz and other improvements #143

Merged
merged 11 commits into from Dec 12, 2018
Merged

Conversation

@jayqi
Copy link
Collaborator

@jayqi jayqi commented Dec 4, 2018

Lots of changes, but I think they make the reports a lot more user-friendly.

Graph viz

  • Removed clustering for orphan nodes. It wasn't really helpful, and it broke all the time. It used the visnetwork::visGroups functionality, preventing other things from using it. Closes #102 as it is no longer #relevant.
  • Refactored AbstractGraphReporter to use visNetwork::visIgraphLayout instead of separately explicitly using igraph to compute a layout and passing coordinates to visNetwork. The new implementation is simpler and doesn't require hard-coded aliases to available layouts.
  • New function igraphAvailableLayouts returns available igraph layout options.
  • Changed defaults for layout and moved some of the options out of AbstractGraphReporter to the specific reporters:
    • AbstractGraphReporter defaults to layout_nicely without visHierarchicalLayout
    • DependencyReporter and InheritanceReporter defaults to layout_as_tree and visHierarchicalLayout
    • FunctionReporter defaults to layout_with_graphopt without visHierarchicalLayout. This seems to work well for large, highly interconnected graphs like function networks tend to be.
  • InheritanceReporter will now default to coloring nodes by class type
  • Graph viz automatically adds a legend if the node color field specified is a character or factor.
  • Added dropdown menu to graph viz that lets you select a node by name. Resolves #132.

Misc

  • Reverses DependencyReporter and FunctionReporter to have convention dependent -> independent. This is how dependency works in UML. Closes #131.
  • Adjusted package reports to have responsive width (set to 80%). Set knitr chunks to fill available space.
  • Fixed a bug where InheritanceReporter errors out if there are parent classes external to the package.
@jayqi jayqi requested review from jameslamb and bburns632 Dec 4, 2018
Copy link
Member

@jameslamb jameslamb left a comment

Looks great Jay!! These are really good changes. I love the dropdown to focus on one node.

I have two recommendations:

  • don't export that igraph thing, just make it a part of documentation / messages
  • you need to update vignettes/pkgnet-intro.Rmd to be sure it's consistent with the new directionality
R/AbstractGraphReporter.R Outdated Show resolved Hide resolved
@jayqi jayqi force-pushed the jayqi:feature/graph_viz branch from f73dc49 to 8ea9c24 Dec 4, 2018
Copy link
Member

@bburns632 bburns632 left a comment

@jayqi This is an excellent PR. I love the viz changes, especially the node selection and functional layout changes. The vignette and pkgdown changes render well, but I have some questions on the content before publish.

tests/testthat/testdata/milne_function_edges.csv Outdated Show resolved Hide resolved
vignettes/pkgnet-intro.Rmd Show resolved Hide resolved
vignettes/pkgnet-intro.Rmd Outdated Show resolved Hide resolved
R/AbstractGraphReporter.R Show resolved Hide resolved
@bburns632
Copy link
Member

@bburns632 bburns632 commented Dec 5, 2018

Oh @jayqi , and you missed a little edit right here: https://github.com/UptakeOpenSource/pkgnet/blob/master/DESCRIPTION#L9 :)

DESCRIPTION Show resolved Hide resolved
@jayqi jayqi force-pushed the jayqi:feature/graph_viz branch 5 times, most recently from a33c396 to f9d3947 Dec 5, 2018
@jayqi jayqi force-pushed the jayqi:feature/graph_viz branch from 7b69e4e to c7579cc Dec 9, 2018
@codecov-io
Copy link

@codecov-io codecov-io commented Dec 9, 2018

Codecov Report

Merging #143 into master will decrease coverage by 2.02%.
The diff coverage is 53.92%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #143      +/-   ##
==========================================
- Coverage   86.62%   84.59%   -2.03%     
==========================================
  Files          10       10              
  Lines         897      909      +12     
==========================================
- Hits          777      769       -8     
- Misses        120      140      +20
Impacted Files Coverage Δ
R/PackageFunctionReporter.R 92.94% <100%> (+0.1%) ⬆️
R/PackageDependencyReporter.R 83.46% <100%> (+0.96%) ⬆️
R/PackageInheritanceReporter.R 73.95% <22.22%> (-5.59%) ⬇️
R/AbstractGraphReporter.R 70.73% <46.66%> (-7.21%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 70a3857...c7579cc. Read the comment docs.

@jameslamb
Copy link
Member

@jameslamb jameslamb commented Dec 11, 2018

@bburns632 can you take another look at this? I think it's ready to go.

@bburns632
Copy link
Member

@bburns632 bburns632 commented Dec 11, 2018

@bburns632
Copy link
Member

@bburns632 bburns632 commented Dec 12, 2018

@jayqi excellent edits in the vignette. Thank you for going through the extra trouble there. Great PR.

@jameslamb
Copy link
Member

@jameslamb jameslamb commented Dec 12, 2018

@bburns632 @jayqi I re-reviewed. Looks great!

I re-read the vignette (per @bburns632 's request)...I agree with the recent change. "dependency"-"dependent" language is much clearer than what we had before.

fatntastic job

@jayqi jayqi merged commit b147a75 into uptake:master Dec 12, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.