Bugfix Nodes Table Merging

[bburns632]

The Issue

Node level statistics are being merged out of order causing issue #165.

As currently written, node statistics are merged under this paradigm:

            outNodeDT <- self$nodes

            outDegreeResult <- igraph::centralization.degree(
                graph = pkg_graph
                , mode = "out"
            )

            # update data.tables
            outNodeDT[, outDegree := outDegreeResult[['res']]] # nodes

However, pkg_graph (which drives the order of outDegreeResult) and the nodes table, outNodeDT, have different node orders:

cbind(igraph::vertex.attributes(pkg_graph)$name
+       , outNodeDT$node)
     [,1]            [,2]           
[1,] "baseballstats" "baseballstats"
[2,] "base"          "methods"      
[3,] "methods"       "stats"        
[4,] "utils"         "graphics"     
[5,] "stats"         "base"         
[6,] "grDevices"     "utils"        
[7,] "graphics"      "grDevices"    

This causes the mismatch seen in #165.

The Fix

Utilizing the existing method update_nodes and some minor manipulation fixes this issue.

The Tests So It is Not Merged Again

Units tests have been added to:

1. confirm all expected statistics are calculated
2. For one external package dependency (stats in baseballstats), confirm node level statistics are calculated and assigned correctly
3. For one internal function dependency (slugging_avg in baseballstats), confirm node level statistics are calculated and assigned correctly
4. For one object (KingOfTheEarth in milne), confirm node level statistics are calculated and assigned correctly
5. For external package dependency network, confirm network level statistics are calculated and assigned correctly
6. For internal function dependency network, confirm network level statistics are calculated and assigned correctly
7. For object inheritance network, confirm network level statistics are calculated and assigned correctly

Note, changes to these unit tests will be required when:

• baseballstats or milne is changed
• a node level statistic is added
• a network level statistic is added
• igraph changes any of their functions affecting the calculations (unlikely)

[codecov-io]

Codecov Report

Merging #166 into master will increase coverage by 5.81%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #166      +/-   ##
==========================================
+ Coverage   84.69%   90.51%   +5.81%     
==========================================
  Files          10       10              
  Lines         915      938      +23     
==========================================
+ Hits          775      849      +74     
+ Misses        140       89      -51

Impacted Files	Coverage Δ
R/AbstractGraphReporter.R	84.64% <100%> (+13.91%)	⬆️
R/CreatePackageReport.R	100% <0%> (+1.75%)	⬆️
R/PackageInheritanceReporter.R	100% <0%> (+26.04%)	⬆️

---

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 1ac65d8...983f382. Read the comment docs.

[jayqi]

LGTM. Thanks for adding the unit tests.

As an FYI @bburns632, I had been toying a while ago with a major refactor of how we handling the igraph object and the different node and graph measures. Here: https://github.com/jayqi/pkgnet/blob/feature/graph-refactor/R/AbstractGraphReporter.R#L305-L629

It's a pretty major change and I didn't want to open a PR before we put out 3.2. But I think it makes things more modular and potentially easier to test.

Looks like I had implemented my node measure assignment to protect against this bug as well. https://github.com/jayqi/pkgnet/blob/feature/graph-refactor/R/AbstractGraphReporter.R#L320-L335

[jameslamb]

Nice work! Really elegant fix, and I'm confident the tests you wrote will prevent us from reintroducing this bug. Well done.

[bburns632]

@jayqi I'm looking forward to that upcoming refactor. It looks clean and probably easier to test. Thanks for holding off for the moment.