-
Notifications
You must be signed in to change notification settings - Fork 36
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
Major Refactor to GraphReporters #181
Conversation
@jayqi some initial feedback on the items you called out. Network vs Graph: I believe we should use the term Here's snippet from the Graph Theory wiki page:
and one from the Network Theory wiki page:
Given the extent of this refactor, this would be a good time to change the use of these terms in this package. Report Header Change: Change to * calculate_default_measures |
@jayqi, @jameslamb, and I had the luxury to meet IRL regarding this PR. With regard to the three items above, the consensus is:
|
411a03a
to
8f2509c
Compare
Codecov Report
@@ Coverage Diff @@
## master #181 +/- ##
==========================================
+ Coverage 90.51% 91.52% +1.01%
==========================================
Files 10 11 +1
Lines 938 920 -18
==========================================
- Hits 849 842 -7
+ Misses 89 78 -11
Continue to review full report at Codecov.
|
@bburns632 @jameslamb --- This PR is ready for final review! |
Can you update the copyright year in footer.html please? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jayqi looks good. I have two suggested changes:
- sort the node tables in some fashion (they are not currently sorted by default).
- minor copyright year update
Also, I have a question on the naming of some active bindings in the function reporter.
report_markdown_path = function(){ | ||
system.file(file.path("package_report", "package_function_reporter.Rmd"), package = "pkgnet") | ||
}, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jayqi Why are there R6 active bindings saved in package function reporter and not active bindings for S4 or R5? Seems odd to build active bindings for only one of many possible types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's because they're not supported by Function Reporter. In any case, I'm going to move those bindings into private methods. I originally did it that way because I wanted to take advantage of active bindings' clean interface. But honestly I don't think users would or should need to use these methods, and they make the interface more complicated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I opened #197 to call out that Function Reporter doesn't support S4 and RC yet.
And I've removed these active bindings. ✅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a few comments. Overall looking good. One thing I couldn't figure out from the diff...can you give me a code snippet with the "main" user workflow with pkgnet
? I'm still struggling to understand whether people now have to use a method call on the reporters to get more statistics or not.
@@ -19,7 +32,7 @@ None | |||
* Revised unit test setup and teardown files to enable devtools::test() to work as well as CRAN server testing ([#167](https://github.com/UptakeOpenSource/pkgnet/pull/167)) | |||
|
|||
## BUG FIXES | |||
* Corrected node statisitcs table merging error ([#165](https://github.com/UptakeOpenSource/pkgnet/issues/165), [#166](https://github.com/UptakeOpenSource/pkgnet/pull/166)) | |||
* Corrected node statistics table merging error ([#165](https://github.com/UptakeOpenSource/pkgnet/issues/165), [#166](https://github.com/UptakeOpenSource/pkgnet/pull/166)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you make this a separate PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really? I don't want to screw with git to undo committing this. I also disagree that this kind of thing needs to be a separate PR. There is no discussion to be had, and setting the bar that high for fixing this kind of small typo makes it less likely that they get fixed. This file has no code and fixing this has no risk of breaking anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"screw with git to undo committing this" --> that is not what I'm recommending. If you made another PR it would get merged immediately (since it's such a small and non-controversial change) and when you rebased this PR to master the diff would disappear with no conflict.
It's fine for now because it seems this PR is close to being merged anyway, but in general I disagree with "setting the bar that high for fixing this kind of small typo makes it less likely that they get fixed". I dislike the pattern of long-lived large PRs picking up other small, unrelated changes because it means that anyone else doing development on the project will be waiting on those changes unnecessarily.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, that's fine. I have had some bad experiences in the past where the rebase still gives me conflicts, but maybe that was just bad luck and I'm overcompensating.
This is the bit I have in the vignette:
Basically right now, you can't add non-default measures into You'd need to take your reporter, whether from directly instantiation or via |
I think this is ready to go!
New AbstractGraph and DirectedGraph classes:
These objects will encapsulate all of the functionality related to the graph model of the package networks, including methods for calculating node and graph measures. They slot into the
pkg_graph
in place of the igraph object.AbstractGraph base class
UndirectedGraph
,BipartiteGraph
(though I can't think of great reporters that use them. Maybe a package social graph with authors as edges?)Calculating measures
node_measures
andgraph_measures
.graphBetweenness
.node_measures()
andgraph_measures()
are used both to retrieve and to calculate specified measures.default_node_measures
anddefault_graph_measures
ofDirectedGraph
.Changes to individual measures
outSubgraphSize
andinSubgraphSize
have been replaced byrecursiveDeps
andrecursiveRevDeps
, which are comparatively decremented by 1 so as to not count the node itself. Resolves inSubgraphSize and outSubgraphSize : Should node include itself in count? #191.inCloseness
,graphInCloseness
,authorityScore
,graphAuthorityScore
outBetweenness
to justbetweenness
. There is only one version of betweenness for directed graphs (pretty sure, and igraph doesn't a flag for that), and I don't think it makes sense to calculate undirected betweenness on a directed graph.Graph Reporter Refactor
graph_class
that must be assigned with aAbstractGraph
class constructor.pkg_graph
now stores theDirectedGraph
object. Accessing the igraph object would need:reporter$pkg_graph$igraph
AbstractGraphReporter
: new methodcalculate_default_measures
that replaces the oldcalculate_network_measures
.calculate_test_coverage
inFunctionReporter
has been moved into this method.get_summary_view
and elsewhere.calculate_default_measures
is now explicitly called in each reporter's .Rmd file to account for the change.network_measures
active binding now concatenates the pkg_graph's graph measures with any non-graph-related network measures. The only reporter currently with non-graph-related network measures is the FunctionReporter, which has coverage-related aggregate measures.Unit Test Overhaul
test-XReporter-class.R
tests the functionality of the class, andtest-XReporter-network.R
does expected value testing of the nodes and edges of the test packages against stored .csvs intests/testthat/testdata
names
. This will include inherited members and is ultimately what is more relevant to what a user sees.setup-logger.R
andteardown-logger.R
. Resolves Remove duplicated logger silencing #170.Other:
set_package
. Resolves Proposal: Prevent set_package after a package has already been set #106.reset_cache
method. Resolves "Resetting cached network information" printed for new PackageDependencyReporter and PackageFunctionReporter #70set_package
andcalculate_default_measures
return self invisibly, enabling method chaining. Resolves Support method chaining #151.InheritanceReporter
: Moved node and edge extraction into private methods to separate the functionality from the active bindings, whose job is to have a read-only view. I think this makes the code easier to read.FunctionReporter
: Removed methodextract_network
which was unnecessarily wrappingextract_nodes
andextract_edges
.get_summary_view
was consolidated intoAbstractGraphReporter
since it seemed like it was the same code copy-pasted.package_dependency_reporter.Rmd
. All blocks now instead haveerror = TRUE
which should continue rendering on error but also print the error.