Personalized page rank - #502
Conversation
|
Compiler-warning counts vs
|
Becheler
left a comment
Becheler
left a comment
There was a problem hiding this comment.
Thanks again for so much work ! 🙏🏽
I think we can reach a leaner PR with simpler API surface by moving some things from the library to the user. It has greater generality and does not overwhelm the library code.
Let me know what you think, 🚀
Arno
|
Hello again both and thank you for looking through the PR. 😄 All the points @Becheler raises make a lot of sense. So, if I understand correctly, algorithms should refrain from adding domain knowledge in the form of DSL-like accompaniments, as these can just be presented in the documentation. Did I get this right? If my understanding is correct, then perhaps all overloads that automatically construct a convergence manager should also be removed, as those versions use domain-backed rules. In that case, the convergence class can be removed altogether too. Thus, after removing everything else other the iteration helpers (as well as Done personalized_page_rank(g, weight_map, personalization_map, rank_map, done, damping, rank_map2);
Done personalized_page_rank(g, weight_map, personalization_map, rank_map, done, damping);There could be an argument to, for historical reasons (mainly evoking the idea of non-personalized PageRank) also have the next signature for Markovian normalization while keeping the Done personalized_page_rank(g, personalization_map, rank_map, damping=0.85); |
|
You're welcome @maniospas ! 🤗 Let's say it this way: for a first PR in uncertain weather (BGL has more steam power these days, but many directions are left uncertain/unexplored) we should take minimal risk and avoid baking opinionated domain policy into the API (it makes your code more powerful, not less!).
Does it make sense ? |
| auto convergence1 = graph::rank_convergence(max_iters, 1.E-9); | ||
| convergence1 = graph::personalized_page_rank(g, weight, personalization_map, rank_map, convergence1, 0.9); | ||
|
|
||
| std::cout << "ended after "<<max_iters-convergence1.iters << " iterations\n"; |
There was a problem hiding this comment.
For last round of review: please refrain from using standard output in tests. Standard outputs are brittle (can't be automatically checked in CI) and make poor non-regression tools.
If the output is predictible, then use assertions: breaking an assertion will stop the CI, breaking an output will not break the CI 😉
There was a problem hiding this comment.
That being said, for a later documentation PR, the examples.cpp scripts can completely use standard output and assertions, as the output is run in CI and integrated into the doc: see https://491.graph.prtest3.cppalliance.org/graph/algorithms/metrics/page_rank.html
But unit tests and examples serve different purpose:
- unit tests should always fail if something expected is broken
- unit tests should use a dedicated logger if something can't be expected/guaranteed (that's one more discussion we have on BGL rn)
- examples should use (static_)assert to simplify reading (it's easier to read
assert(expected == computed)than reading the output to visually compare the two - examples should use output for brief, visual proofs/illustrations: it may be worth relying on visual checks if a formal check would double the size of the example script.
PS: yes this should go to the contribution guidelines, I just added an appendix section about it in #495 : thank you 🙏🏽
|
Hi @maniospas ! Thanks for editing the files, I think it's perfect like this 🏆 For the last step, it should be pretty straightforward now: just scan what the algorithm requires on the templated types (graph concepts, property map concept), expand a bit the test if you can think of corner edges (damping factor definition domain violation, different graph types, different property maps). You don't have to go full 500 loc test file, but maybe defining two test functions outside the main and calling them with e.g adjacency list, adjacency matrix, csr, would already strengthen your implementation and guarantee it works across a reasonably variable range of data structs, something like: template <typename Graph>
void test_basic(const Graph& g)
{
// basically what you already have I believe
}
template <typename Graph>
void test_corner_cases(const Graph& g)
{
// just add a few corner cases you can think of ? If any, no worries
}
int main()
{
typedef adjacency_list<vecS, vecS, directedS> AdjList;
typedef adjacency_matrix<directedS> AdjMatrix;
typedef compressed_sparse_row_graph<directedS> CSR;
// build the same small graph for each backend, then run both suites
{
AdjList g; /* add_edge(...) */
test_basic(g);
test_corner_cases(g);
}
{
AdjMatrix g(/* n */); /* add_edge(...) */
test_basic(g);
test_corner_cases(g);
}
{
CSR g(/* edges_are_unsorted, begin, end, n */);
test_basic(g);
test_corner_cases(g);
}
return boost::report_errors();
}Intuitively I would think doubling your test file size would be ok, tripling it would be overdoing it (just for an order of magnitude) 😄 |
|
Hello @Becheler again and many thanks for the feedback and helping me with this PR. 😄 I am pretty happy where the whole thing ended up, too. I added the last overload because it is ~80% what people new to node ranking will want to use, though it is sufficient for perhaps only ~20% of end-products (numbers not real: trying to convey my intuition here, in case it is unwanted). By the way, I took the liberty of having the remaining iterations and tolerance to be public struct fields to allow greater versatility without adding any class methods for convergence. I am not sure if you noticed and are ok with it, so mentioning it here. With regards to asserts, I can see even negative damping and negative edge weights to be viable options, so I would ideally not have any numerical restrictions. Even zero normalization yielding NaNs could be meaningful information, in my opinion. At least, for me, it feels that if there is no error convergence there also should be no error for more "exotic" values that still make sense. Perhaps a good compromise assert guard would be for dampening to be in the range [-1,1] (the negative values are needed to be able to express high-pass gradients via the Laplacian) but I am not sure if adding such a restriction is meaningful. Is there a chance the algorithm can expose warnings instead of errors? Perhaps this could be the purview of feature visitors once the respective discussion is stabilized. Working on the concept asserts and tests now. Will ping you again for the result, because I am not confident about my understanding of available concepts. For tests, I will add the breadth you mention - there should be no problem staying in a small volume while creating a matrix of option combinations. A final question I have is whether accompanying documentation should be part of this PR or be provided separately? |
Perfect, it seems amazing for a first step ! 👍🏽
Ooh this escaped my attention, thank for mentioning it. From a self-documenting perspective, I think we want them to remain
Stuff like that. It's lot of extra words to explain that the only public contract the user should fulfill is to expose only: bool operator()(const RankMap& current, const RankMap2& previous, const Graph& g)and nothing more. You can mark them
I will definitely trust your expert knowledge here !
Nobody is, so don't hesitate asking questions here or in the discussions ! Just know you can probably finish writing the tests and the code without concepts, those come at the very very end to guarantee you don't run into wrong operations because you passed e.g. a property map that can only be read (e.g. a lambda returning 42 for every vertex: ReadablePropertyMap) when you also need to write to it (ReadWritePropertyMap). Concepts will not change your code behavior or make tests fail: they will just give cleaner compile error messages if users did something wrong so that would be last thing I'd write.
Another cross-fire I don't want you to get caught into 😆
|
|
Hello @Becheler and apologies for the delay; I had some personal issues. Letting you know that I will now be resuming work on this PR assuming the previous status. Nonetheless, if some things have changed or been stabilized in the interim (e.g., doc format) I would be happy to adjust/include them. |
|
Hi @maniospas ! 😄 I was considering asking you if everything was ok ! I am glad you are back and I hope you are doing well. There were positives to this delay, mainly that the documentation PR was merged, so writing the documentation page is now easier than ever! But that will be the last step so you don't have to worry about it yet ! Welcome back, happy to help pick things up if you need me to 🎉 |
|
Hello again and thanks for the kind of words. :-) Tried to add concept asserts in the main algorithm, if you can check them, as well as a first matrix of directed graph tests (will add directed tests too to compare normal PageRank with a bidirectional run). Not sure at all if I checked against the correct concepts. Also added an assert on the damping after all, because it was revealed during edge case testing that certain unstable divergent computations somehow do not preserve symmetry, whereas I was expecting the computations to create the overflows but mathematically preserve subgraph isomorphisms even in edge cases. Tangent: Not certain why exactly the isomoprhism property is not preserved; I thought the computations would at worst become infinity per IEEE754 and then nan, but perhaps there's a numerical underflow in a denominator. Thus, I think the likely "fix" would be to traverse nodes in rank order but this creates P.S. Random feedback on navigating the documentation on concepts: I think I am kind of missing a place to see all concepts side-by-side/list/dependency graph (whatever makes sense) linked from all concept pages, because their descriptions are really great at explaining differences but it was kind of burdensome to keep switching pages to grok the details. |
|
Boost dependency footprint vs Header-inclusion weights (graph files pulling each direct dependency in):
Transitive Boost modules: 68 → 68 (0) |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #502 +/- ##
========================================
Coverage 92.44% 92.45%
========================================
Files 392 394 +2
Lines 28214 28309 +95
Branches 8008 8047 +39
========================================
+ Hits 26083 26172 +89
- Misses 2026 2031 +5
- Partials 105 106 +1
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
|
I will have a look thank you ! In the meantime, would you be able to sync your fork with the boost repo, then rebase your PR branch on develop so the PR shows only your commits ? I don't know what happened but suddenly 170 commits and 959 modified files were dropped on the PR and it's hard to track what changed :) Thank you ! Edit: I don't know if it's too late to do so. It looks like develop was merged into the PR branch rather than the PR branch rebased on top of develop :) Also make a local copy before doing e.g. Edit2: IIUC it's too late for a clean rebase. I think you would want something like this: create a new branch from develop, move to it your own commits, then repoint to the PR branch Of course double check what I'm saying ! I would not want your work to be erased ! <3 |
|
Ah, sorry, my bad. Did not notice. I used GitHub's sync function, thinking that I needed to be up to date to keep compatibility with any breaking changes (if any). 😓 I'll tinker somehow. The long chain of commits above are likely those that have been merged into develop in the interim, and have no new work by me. Mine are just these two. 
If I am unable to do anything, I will tell you; perhaps the worst case is to delete the fork and PR and create a new one, but I'd rather not resort to such ... shameful measures. XD |
|
Ahhah no worries we will figure this out, this damn "Sync fork" button is a very common trap! It does a merge (not a fast-forward) when the branch has diverged, and if it gets pointed at the branch the PR is built on, it drags in all of upstream's interim history as merge noise... 😅 I don't even know why Github offers this lol
So it may be even worse than anticipated: if you merged your feature branch on your develop branch, it probably means your own develop ( I suggest something around: |
62e7da6 to
c6163f8
Compare
|
Update: I think I fixed it. Sorry for spamming - thought it would be harder. Yeah, as you found out also concurrently, just cherry-picked the commits and it looks fine (thankfully, they were only a few). The new ones are |
|
Amazing @maniospas ! Thanks for being so quick ! Let me have a look at those new commits ! 🙏🏽 |
Becheler
left a comment
Becheler
left a comment
There was a problem hiding this comment.
All good, thanks again !
There are just more idiomatic details you sincerely had no way to be aware of. I believe I mentioned them somewhere in a contribution guideline, but there is so much to learn that you have to go through it once or twice and get bitten before to get it right 😮💨
| #include <boost/property_map/property_map.hpp> | ||
| #include <boost/graph/graph_traits.hpp> | ||
| #include <boost/graph/properties.hpp> | ||
| #include <boost/graph/iteration_macros.hpp> |
There was a problem hiding this comment.
weird display, I meant the iteration_macros header is dead.
| #include <boost/graph/properties.hpp> | ||
| #include <boost/graph/iteration_macros.hpp> | ||
| #include <boost/graph/overloading.hpp> | ||
| #include <boost/graph/detail/mpi_include.hpp> |
There was a problem hiding this comment.
weird display, I meant the mpi_include header is dead (and the overloading too, see further)
| #include <boost/graph/detail/mpi_include.hpp> | ||
| #include <boost/property_map/function_property_map.hpp> | ||
| #include <vector> | ||
|
|
There was a problem hiding this comment.
Several includes seem to be missing:
#include <boost/range/iterator_range.hpp> // boost::make_iterator_range
#include <boost/concept/assert.hpp> // BOOST_CONCEPT_ASSERT
#include <boost/graph/graph_concepts.hpp> // IncidenceGraphConcept, VertexListGraphConcept
#include <cmath> // std::abs
#include <cassert> // assert
#include <cstddef> // std::size_t| Done done, | ||
| typename property_traits< RankMap >::value_type damping, | ||
| RankMap2 rank_map2 | ||
| BOOST_GRAPH_ENABLE_IF_MODELS_PARM(Graph, vertex_list_graph_tag)) |
There was a problem hiding this comment.
The BOOST_GRAPH_ENABLE_IF_MODELS_PARM(Graph, vertex_list_graph_tag)) should be removed, it has no usefulness here (its for distributed stuff only, AI probably copies it by mistake from the Page Rank source code, and should never appear in public interfaces anyway). Once it's gone, the <boost/graph/overloading.hpp> becomes dead too and should be gone 😺
| bool to_map_2 = true; | ||
| do | ||
| { | ||
| typedef typename graph_traits< Graph >::traversal_category category; |
There was a problem hiding this comment.
please prefer the using category = graph_traits< Graph >::traversal_category modern equivalent. Typedefs are things of the past 😉
| BOOST_CONCEPT_ASSERT(( boost::IncidenceGraphConcept<Graph> )); | ||
| BOOST_CONCEPT_ASSERT(( boost::VertexListGraphConcept<Graph> )); | ||
| BOOST_CONCEPT_ASSERT(( boost::ReadablePropertyMapConcept<WeightMap, Edge> )); | ||
| BOOST_CONCEPT_ASSERT(( boost::ReadablePropertyMapConcept<PersonalizationMap, Vertex>)); |
There was a problem hiding this comment.
This is underconstrained, as PerzonalizationMap uses put to write stuff to it to normalize in place (l. 166) and restore magnitude (l.198).
So you'd want to use ReadWritePropertyMapConcept that allows both for get and put
| BOOST_CONCEPT_ASSERT(( boost::ReadablePropertyMapConcept<WeightMap, Edge> )); | ||
| BOOST_CONCEPT_ASSERT(( boost::ReadablePropertyMapConcept<PersonalizationMap, Vertex>)); | ||
| BOOST_CONCEPT_ASSERT(( boost::ReadWritePropertyMapConcept<RankMap, Vertex> )); | ||
|
|
There was a problem hiding this comment.
But rankMap2 is completely unchecked. You will want to add:
BOOST_CONCEPT_ASSERT(( boost::ReadWritePropertyMapConcept<RankMap2, Vertex> ));
| BOOST_CONCEPT_ASSERT(( boost::ReadWritePropertyMapConcept<RankMap, Vertex> )); | ||
|
|
||
|
|
||
| assert (damping>=-1.0 && damping<1.0 && "Damping outside the closed-open range [-1.0,1.0) could induce numerical instability."); // non-inclussive upper limit is deliberate |
There was a problem hiding this comment.
The convention is to rather use the Boost version of assert, that is more flexible that the standard (users can reroute it and do all sorts of tricky things):
#include <boost/assert.hpp> // you will need to add this
....
BOOST_ASSERT_MSG(damping >= -1.0 && damping < 1.0, "Damping outside the closed-open range [-1.0,1.0) could induce numerical instability.");
| assert(convergence.get_remaining_iters()<max_iters); // ran | ||
| assert(convergence.get_remaining_iters()>0 || (damping<0.0)); // converged (derivatives may not converge) | ||
| assert((ranks[0]<ranks[1]+0.1) || (damping<0.0) || damping>1.0); // holds for all graphs given low-pass damping | ||
| assert((ranks[0]!=ranks[1]) == (damping!=0.0)); // but not the same normally, the same 1.0 personalization if damping is zero | ||
| assert(ranks[0]==ranks[2] || (damping<=-1.0)); // equal due to symmetry, even under non-convergence and floating coarseness | ||
| assert(std::abs(ranks[0]-ranks[2])<1.E-14); // approximately equal in cases where even addition order matters | ||
| assert((ranks[0]<0.0)||(ranks[1]<0.0)||(ranks[num_vertices(g)-1]<=0.0)||(damping>0.0)); // ensure that negative flows are possible for negative damping | ||
| } |
There was a problem hiding this comment.
If those tests are ever built in release mode, all those asserts will disappear 😄
That's a reason to use Boost instead, with the following method:
#include <boost/core/lightweight_test.hpp> // you need to include this
...
BOOST_TEST(convergence.get_remaining_iters() < max_iters); // tests it's true
...
return boost::report_errors(); // at end of mainThere was a problem hiding this comment.
Also I guess you are aware that the entire bidirectional_graph_tag path is untested ? 😉
2 participants
Before submitting
developbranch.Type of change
Does this PR introduce a breaking change?
What this PR does
Provides an implementation of personalized PageRank following the motivating issue's outline for the provided interface.
Motivation
Refs #493
Testing
personalized_pagerank_test.cppwas added to tests. This runs two node scoring scenarios on a graph with well-understood structure, where assertions are added for equality between isomorphic nodes and certain interesting numerical comparisons checked to hold. The build command for local testing wasg++ -std=c++14 -I./boost -o test test.cpp -O2.Checklist
b2in thetest/directory).Documentation change not applicable, but likely new documentation should be added to reflect the introduced algorithm.