This commit refactors the `analysis/weights` module so that there's
a single top-level type called `Weights` which includes all of the
user-specified weights for running PageRank. This includes both manually
specified type weights, and per-node weights.
In contrast to the old weights module which had weighted types (i.e. the
type bundled with the weight), this only records the weight choice
(keyed by address). Similarly, the map is empty unless the user has
explicitly chosen a weight. This has a few advantages:
- For planned work on serializing weights, it's convenient that
there's a single unified object that bundles all the weights.
- For planned work on serializing weights, it's convenient that we only
record actual choices, not the default values (defaults are provided
within the WeightsToEdgeEvaluator). This way, we don't need to manually
filter out the default weights for serialization. (We want users who
have not made any choice about the type weights to automatically get new
type weightings we publish in sourcecred/sourcecred).
- Overall, the WeightConfig data piping has become a lot simpler.
While making these changes, I threw away the PluginWeightConfig
component, instead inlining it in the top-level WeightConfig. I also
moved responsibility for the WeightConfig out of the PagerankTable, and
into the containing App; the WeightConfig is created and wired by the
App, and then passed as a whole component into the PagerankTable via
props. This means that PagerankTable does not need to get a bunch of
extra props for piping the state changes into WeightConfig.
I also threw away some frontend test code for the WeightConfig
component. Over the course of the past few months, I've observed that
the value of a lot of the frontend-wiring-testing is pretty low (i.e.
they are not catching issues, because Flow catches them) and the cost of
maintaining the testing is high. I'm now inclined to believe that we
shouldn't be testing routine component wiring via explicit tests,
because it's very easy to verify that the behavior is correct by
interacting with the UI, and the tests are expensive to write and
maintain.
For "core logic" (e.g. behaviors of types and data structures,
algorithms, etc) I continue to believe testing is very valuable. One
heuristic I'll start applying is: "can I achieve high confidence that
this code is correct, without tests, using little effort by manually
inspecting the frontend?". If the answer is yes, I'm unlikely to write
tests.
Test plan: `yarn test` passes, and the UI continues to function,
including changing manual and type level weights and observing cred
changes.
Node and Edge types are increasingly important in SourceCred, as we use
them to decide what weights to provide, what description logic to use,
etc. In #631, we hit a bug around not finding a type matching a
particular node, and in #640 I responded by creating a
"FallbackAdapter", which matches any node, and a
"Static/DynamicAdapterSet", which are abstractions for finding adapters
in a way that guarantees the presence of the fallback adapter.
I think this solution is actually pretty brittle. It only works if we
are disciplined in only ever accessing node and edge types using a
context that included the FallbackAdapter, and it requires us to
centralize all of the "type-not-found" logic in one place (the fallback
declaration) irrespective of what the locally appropriate solution is.
Looking back at #631, the core issue was that we had a getter that
promised to always give a matching type and adapter, rather than
returning a possibly undefined adapter. We fixed this by attempting to
ensure that there always would be a matching adapter. I think it would
be better to have the methods honestly report that the adapter might be
null/undefined, and let the client code handle it appropriately.
This commit implements that change. It's motivated by me being
frustrated at the fallback adapter while doing other refactoring.
Test plan:
Unit tests have been updated and `yarn test` passes. Also, I
experimentally removed the Git plugin from the list of adapters for both
the backend and frontend, and verified that the frontend UI and the
pagerank and export-graph commands still worked.
This refactors the weights module (and downstream consumers) so that
rather than tracking forwardWeight and backwardsWeight as separate
fields, we have an `EdgeWeight` type with a forwards and backwards
property. I feel this makes the code a little cleaner and wanted it a
few times while doing other refactors in this module.
Note that the graphToMarkovChain module has a distinct `EdgeWeight` type
which serves a similar purpose but happens to have different field
names. I've added a TODO to rename those fields for consistency.
Test plan: Since this is slight data structure re-organization, no new
tests are required; that `yarn test` passes is sufficient.
This pull request adds a weight slider to every NodeRow in the explorer,
enabling the user to manually set a weight for that node. The weights are
multiplicative with the type level weights, so that they can be changed
independently (e.g. you can have a comment that is weighted 2x higher than
regular comments, but still have comments get a low weight in general).
This pull coordinates a number of different changes across the codebase, all of
which are tested:
Adding support for manual weights in the weights and
weightsToEdgeEvaluator modules.
Modifying pagerankTable.TableRow so that it can show a slider in the second
column.
Adding piping for manual weights into the PagerankTable shared props, and
into the explorer app
Adding the slider to the NodeRow class that displays the current weight,
and can trigger the upstream weight change
Ensuring that the runPagerank call in the explorer actually uses the manual
weights
At present, there is no way to save these weights (they are ephemeral in the
frontend) and so this is clearly a prototype/tech demo level feature rather
than being ready for real usage. Correspondingly, CLI pagerank command always
uses an empty set of manual weights. I plan to remedy this in a follow-on pull
request.
Test plan: Run the included unit tests (yarn test) and also spin up the UI,
verify that it visually looks good in both Firefox and Chrome, and verify that
changing the weights and then re-running PageRank actually causes the cred of
the modified node to change.
Review plan: In addition to carefully reading the code, ensure that all of the
changes described a few paragraphs up are actually tested.
Merge plan: Squash and merge.
Thanks to @s-ben for proposing this feature in Discord, and to everyone
discussing its implications in this Discourse thread.
In #1140 I rename a field in PagerankGraph.ScoredNode, and in #1141 I
added a new test which referenced the field.
I merged them in quick succession, introducing a conflict. This PR fixes
it.
Test plan:
`yarn test` passes.
This commit adds a `setEdgeEvaluator` method to `PagerankGraph`,
and modifies the constructor to use that method. This will allow us to
use PagerankGraph in the explorer UI, so that we can update edge
weights without fully regenerating the graph.
Test plan: I've added new unit tests that verify basic properties of how
the edge weights are getting set and consumed.
It makes more sense that a ScoredNode has an address than that it has a
sub-node (which is an address).
Test plan: It's a simple field rename; `yarn test` suffices
Resolves#1067
Adds the CLI commands:
`sourcecred clear --all` -- removes the $SOURCECRED_DIRECTORY
`sourcecred clear --cache` -- removes the cache directory
`sourcecred clear --help` -- provides usage info
`sourcecred clear` -- prompts the user to be more specific
Test plan:
The unit tests ensure that the command is properly wired into the
sourcecred CLI, including help text integration. However, just to be
safe, we can start by verifying that calling `sourcecred` without
arguments lists the `clear` command as a valid option, and that
calling `sourcecred help clear` prints help information. (Note: it's
necessary to run `yarn backend` before testing these changes)
The unit tests also ensure that the command removes the proper
directories, so there isn't really a need to manually test it,
although the reviewer may choose to do so to be safe.
Although out of scope for unit tests on this function, we can also do
integration tests, to make sure that running the clear command doesn't
leave the sourcecred directory in an invalid state from the perspective of the `load` command.
```js
$ yarn backend;
$ node bin/sourcecred.js load sourcecred/example-github;
$ node bin/sourcecred.js clear --cache;
$ node bin/sourcecred.js load sourcecred/example-github;
$ node bin/sourcecred.js clear --all;
$ node bin/sourcecred.js load sourcecred/example-github;
```
The expected behavior of the above command block is that the load command never fails or throws an error.
@decentralion and I discussed the scenario where `rimraf` errors.
We decided that testing this scenario wasn't necessary, because
`rimraf` doesn't error if a directory doesn't exist, and
rimraf's maintainer suggests [monkey-patching the fs module]
to get rimraf to error in testing scenarios.
Thanks @decentralion for reviewing and pair-programming this with me.
[monkey-patching the fs module]: https://github.com/isaacs/rimraf/issues/31#issuecomment-29534796
This commit integrates an bare skeleton of the odyssey frontend that we
implemented in the [odyssey-hackathon] repository. You can see the
working frontend that we are trying to port over at
[sourcecred.io/odyssey-hackathon/][scio].
The prototype in the other repository has some tooling choices which are
incompatible/redundant with decisions in our codebase (sass vs
aphrodite), and requires some tools not yet present here
(svg-react-loader). This commit includes the build and integration work
needed to port the prototype frontend into mainline SourceCred. The
frontend scaffold isn't yet integrated with any "real" Odyssey data.
One potential issue: right now, every page that is rendered from the
SourceCred homepage is contained within a [homepage/Page], meaning that
it has full SourceCred website styling, along with the SourceCred
website header. The [application][scio] also has a header. Currently, I
work around this by having the Odyssey UI cover up the base header (via
absolute positioning), which works but is hacky. We can consider more
principled solutions:
- Finding a way to specify routes which aren't contained by
[homepage/Page]; maybe by adding a new top-level route
[here][route-alternative].
- Unify the headers for the Odyssey viewer and the page as a whole
(sounds like inappropriate entanglement?)
- Have a website header and also an application header (sounds ugly?)
[homepage/Page]: ee1d2fb996/src/homepage/Page.js
[route-alternative]: ee1d2fb996/src/homepage/createRoutes.js (L17)
Test plan: Run `yarn start`, and then navigate to
`localhost:8080/odyssey/`. observe that a working website is displayed,
and that the cred logo next to the word "SourceCred" is loaded properly
(i.e. svg-react-loader is integrated properly). Observe that there are
no build/compile errors from either `yarn start` or `yarn build`. Also,
observe that the UI looks passably nice, and that if the number of
elements in the entity lists is larger than can be displayed, the
sidebar pane scrolls independently.
The UI was tested in both Chrome and Firefox.
[odyssey-hackathon]: https://github.com/sourcecred/odyssey-hackathon
[scio]: https://sourcecred.io/odyssey-hackathon/
Thanks to @jmnemo, as the implementation is based on [his work].
[his work]: https://github.com/jmnemo/hackathon-event/
This commit puts in a basic data model for the Odyssey plugin. It's
built around the `OdysseyInstance` class, which is basically a Graph
that keeps around descriptions for every node, and ensures that
nodes/edges are typed in accordance with the Odyssey plugin declaration.
In the future, I want to enable instances to declare their own node/edge
types, in which case the instance will assume responsibility for
tracking and serializing the types.
To make the Odyssey plugin a 'proper plugin', I've also added a plugin
declaration, as well as analysis and explorer adapters. I haven't
decided exactly where data for Odyssey instances should be stored, so
for now the plugin adapters always return an example instance which is
based on our experience at the Odyssey hackathon.
Test plan: The instance has unit tests for its logic.
If you want to see what the plugin looks like right now when it's
integrated, you can apply the following diff, and then load the
prototype. It will contain Odyssey nodes (find them using the node type
dropdown). Note that without a seed vector to move cred back to the
values/artifacts, the cred distribution in the Odyssey subgraph is
degenerate; the users are all sinks and have postiive cred scores, but
all the other nodes converge to 0 cred.
diff --git a/src/homepage/homepageExplorer.js b/src/homepage/homepageExplorer.js
index cae4548..48987f1 100644
--- a/src/homepage/homepageExplorer.js
+++ b/src/homepage/homepageExplorer.js
@@ -6,6 +6,7 @@ import type {Assets} from "../webutil/assets";
import {StaticExplorerAdapterSet} from "../explorer/adapters/explorerAdapterSet";
import {StaticExplorerAdapter as GithubAdapter} from "../plugins/github/explorerAdapter";
import {StaticExplorerAdapter as GitAdapter} from "../plugins/git/explorerAdapter";
+import {StaticExplorerAdapter as OdysseyAdapter} from "../plugins/odyssey/explorerAdapter";
import {GithubGitGateway} from "../plugins/github/githubGitGateway";
import {AppPage} from "../explorer/App";
import type {RepoId} from "../core/repoId";
@@ -14,6 +15,7 @@ function homepageStaticAdapters(): StaticExplorerAdapterSet {
return new StaticExplorerAdapterSet([
new GithubAdapter(),
new GitAdapter(new GithubGitGateway()),
+ new OdysseyAdapter(),
]);
}
This commit modifies `PagerankGraph.runPagerank` so that the user can
provide an alpha and seed vector. The seed vector is specified via a map
of weights, which will be normalized into a probability distribution
over all the nodes in the graph. In the event that the map is empty (or
the total weight is otherwise 0), a uniform distribution is created.
To effect this change, a helper function called `weightedDistribution`
has been added (and thoroughly tested) in the `graphToMarkovChain`
module. Then, that function is used in `pagerankGraph.runPagerank`
(along with light testing).
Currently, the default alpha is set to 0, to ensure consistency with the
legacy pagerank implementation in `analysis/pagerank`. Once that has
been replaced with `PagerankGraph`, we can consider changing the defualt
alpha to non-zero (thus removing the need for synthetic self-loops).
I took a different approach in the [odyssey-hackathon repo][commit].
The previous approach was a much more complicated (and fairly redundant)
API, that allowed specifying "NO_SEED", "UNIFORM_SEED", "SELECTED_SEED",
and "SPECIFIED_SEED". I'm much happier with this API and implementation.
[commit]: ed07861073
Test plan: Unit tests included; run `yarn test`.
Summary:
the cred calculation is defined by a Markov Mixing process. By
introducing the seed vector and teleportation parameter alpha, the
Markov mixing process is augmented with a source of cred originating
from the seed vector. The resulting algorithm is the generalized
variation of PageRank, allowing computation of both canonical PageRank
where the seed vector is the uniform distribution and personalized
PageRank where the seed vector is an indicator distribution. It is still
possible to get the simple markov chain solution by setting alpha = 0.
Note that this changes the Markov process state update, but does not
provide updates to the APIs. All existing behavior is unchanged because
alpha is always set to 0.
This is a port of
https://github.com/sourcecred/odyssey-hackathon/pull/3,
which was created during the Odyssey Hackathon.
Test Plan:
Existing tests have been extended to include passing alpha = 0 to
reproduce exisiting test cases for the simple Markov Process. Additional
test cases include
- Verifying that resulting stationary distribution is unaffected by seed when alpha = 0
- Verifying that resulting stationary distribution is precisely equal to seed when alpha = 1
- Verifying that the resulting stationary distribution is linear in the seed vector
- Verifying that the correct stationary distribution is computed for non-zero alpha
- verify that the algorithm converges immediately when the initialDistribution is the stationary distribution
- verify that the changing the initialDistribution does not change the stationary distribution
Paired with @mzargham
Right now PagerankGraph requires that the user choose specific values
for maxIterations and convergenceThreshold when running PageRank.
I also rename `PagerankConvergenceOptions` to `PagerankOptions`.
The motivation is that I want to add future arguments to the same
options dict (e.g. alpha and the seed vector), so the rename is
appropriate, and allowing the options to be unset (and thus inherit
default values) will make the whole API much cleaner as I add more
options.
Test plan: Unit test added. `yarn test` passes.
In [#1128: Add support for seed vectors][#1128], we significantly
increase the number of arguments to
markovChain.findStationaryDistribution. To clean up the invocations, I
added a followon PR (#1129) which converts findStationaryDistribution to
use a `PagerankParams` object instead.
However, I think it will be cleaner to land the PagerankParams refactor
before adding new features in #1128, so I'm making this PR as
pre-cleanup.
Test plan: This is a trivial refactor. `yarn test` passes.
[#1128]: https://github.com/sourcecred/sourcecred/pull/1128
This is a minor refactor so that we use Aphrodite for styling on
HomePage.js. It's not super consequential, but I want to switch to using
Aphrodite more consistently in the codebase, so why not start here.
Test plan:
`yarn test` reveals no errors.
`yarn start` launches a correctly styled frontend.
I also used `build_static_site.sh` and the resultant site is also
correctly styled.
This commit updates the `sourcecred load` command so that it also
automatically runs PageRank on completion.
The implementation is slightly hacky, in that it prints two sets of
task status headers/footers to console, for reasons described in a
comment in the source code. The user-visible effect of this hack can
be seen below:
```
❯ node bin/sourcecred.js load sourcecred/example-github
Starting tasks
GO load-git
GO load-github
DONE load-github
DONE load-git
Overview
Final result: SUCCESS
Starting tasks
GO run-pagerank
DONE run-pagerank
Overview
Final result: SUCCESS
```
It would be good to clean this up, but for now I think it's acceptable.
Note that it is not safe to assume that a PagerankGraph always exists
for repos that are included in the RepoIdRegistry. The repo gets added
to the registry before the pagerank task runs. Consumers that are
loading the `PagerankGraph` can just check that the file exists, though.
Test plan: I've added unit tests that verify that the right tasks are
generated. Most importantly, the snapshot of the results of `sourcecred
load` now include a snapshotted pagerank graph.
(The snapshot was updated via `UPDATE_SNAPSHOT=1 yarn test --full`.)
Further progress on #967.
Across SourceCred usage, we depend on the `SOURCECRED_GITHUB_TOKEN`
environment variable being set. Confusingly, some tests expect
`GITHUB_TOKEN` instead of `SOURCECRED_GITHUB_TOKEN`.
This commit resolves that inconsistency, by having all tests that read
from the environment use `SOURCECRED_GITHUB_TOKEN`. This was already
available as a secret in our CI configuration, so no change is needed
there. (After this merges, we may remove the GITHUB_TOKEN variable from
the environment.)
Test plan: `yarn test --full` passes without the environment variable
set. Also, the following grep produces only innocuous hits.
```
git grep -P "(?<\!SOURCECRED_)GITHUB_TOKEN"
```
This commit refactors the `sourcecred load` CLI command so that it uses
dependency injection, much like the testing setup #1110. This makes it
feasible to test "surface logic" of how the CLI parses flags and
transforms them into data separately from the "piping logic" of invoking
the right API calls using that data.
This is motivated by the fact that I have other pulls on the way that
modify the `load` command (e.g. #1115) and testing them within the
current framework is onerous.
Test plan:
This is a pure refactoring commit, which substantially re-writes the
unit tests. The new unit tests pass (`yarn test --full` is happy).
Note that `yarn test -full` also includes a sharness test that does an
E2E usage of `sourcecred load`
(see sharness/test_load_example_github.t), so we may be confident that
the command still works as intended.
This commit fixes three broken links (two in the README, one in the prototype app) that were still pointing to https://discuss.sourcecred.io/.
Test plan:
Verify that there are no other bad links to the old Discourse location, by running `git grep "discuss.sourcecred.io"`.
This commit adds a new CLI command, `pagerank`, which runs PageRank on a
given repository. At present, the command only ever uses the default
weights, although I plan to make this configurable in the future. The
command then saves the resultant pagerank graph in the SourceCred
directory.
On its own, this command is not yet very compelling, as it doesn't
present any easily-consumed information (e.g. users' scores). However,
it is the first step for building other commands which do just that. My
intention is to make running this command the last step of `sourcecred
load`, so that future commands may assume the existence of pagerank
scores for any loaded repository.
Test plan: The new command is thoroughly tested; see
`cli/pagerank.test.js`. It also has nearly perfect code coverage (one
line missing, the dependency-injected real function for loading graphs).
Additionally, the following sequence of commands works:
```
$ yarn backend
$ node bin/sourcecred.js load sourcecred/pm
$ node bin/sourcecred.js pagerank sourcecred/pm
$ cat $SOURCECRED_DIRECTORY/data/sourcecred/pm/pagerankGraph.json
```
Material progress on #967.
This commit adds a module, `fetchGithubOrg`, which loads data on GitHub
organizations, most notably including the list of repositories in that
org.
The structure of this commit is heavily influenced by review feedback
from @wchargin's [review] of a related PR.
Test plan: This logic depends on actually hitting GitHub's API, so the
tests are modeled off the related `fetchGithubRepo` module. There is a
new shell test, `src/plugins/github/fetchGithubOrgTest.sh`, which
verifies that that the org loading logic works via a snapshot.
To verify the correctness of this commit, I've performed the following
checks:
- `yarn test --full` passes
- inspection of `src/plugins/github/example/example-organization.json`
confirms that the list of repositories matches the repos for the
"sourcecred-test-organization" organization
- manually breaking the snapshot (by removing a repo from the snapshot)
causes `yarn test --full` to fail
- running `src/plugins/github/fetchGithubOrgTest.sh -u` restores the
snapshot, afterwhich `yarn test --full` passes again.
[review]: https://github.com/sourcecred/sourcecred/pull/1089#pullrequestreview-204577637
This pulls the logic for loading a SourceCred graph from disk out
`cli/exportGraph` and into `analysis/loadGraph`. The rationale is that
`exportGraph` is not the only command that wants the ability to load a
graph from the analysis adapters.
The new command has a clean return signature that reports whether the
load was successful, or failed because the graph wasn't loaded, or
failed due to an error in plugin code.
Testing of the loading logic has been moved to `loadGraph.test`, and the
CLI has been refactored so that the loadGraph method is dependency
injected. This allows for (IMO) cleaner testing of the CLI method.
There is one (deliberate) change in behavior, which is that the command no
longer throws an error if no plugins are included; instead it will just
export an empty graph. I don't have a strong preference between the two
behaviors; changing it was just more convenient.
Test plan: New unit tests have been added, and tests of the cli command
have been re-written. As a sanity check, I've verified that the
following sequence still works:
```
$ yarn backend
$ node bin/sourcecred.js load sourcecred/pm
$ node bin/sourcecred.js export-graph sourcecred/pm
```
Nearly perfect code coverage is maintained. One line is uncovered, and
it's the line that injects in the actual graph loading behavior.
This commit adds a `neighbors` method to `PagerankGraph`. This is an
augmented version of `Graph.neighbors`. It returns the base data from
`Graph.neighbors` as well as the score, the edge weights, and the score
contribution. The score contribution basically means how much score was
contributed from the target node by this particular neighbor connection.
When the graph is well-converged, a node's score will be the sum of all
its neighbors' score contributions, as well as the contribution it
received from its synthetic loop edge. So, for completeness sake, I
added another method, `syntheticLoopScoreContribution`, which computes
how much score a node received from its synthetic loop edge. (This value
should usually be close to 0).
You can think of these two methods as providing a replacement for the
`PagerankNodeDecomposition` logic.
Test plan: I've added tests that verify:
- That neighbors returns results consistent with Graph.neighbors
- That neighbors' score contributions are computed correctly
- That neighbors errors if the graph has been modified
- That synthetic score contributions are computed correctly
- That a node's score is the sum of all its contributions
Test plan: Unit tests included. Run `yarn test`.
This commit moves the default Pagerank options out of
`analysis/pagerank` and to `core/pagerankGraph`. This reflects the
gradual migration of core pagerank logic into `pagerankGraph`.
Test plan: `yarn test` should suffice. It's a trivial change.
* Show tooltips in weightConfig UI
* Updated to pass checks from prettier
* Updates unit tests to check WeightSlider descriptions
* Update CHANGELOG.md to reflect PR #1081
* Add CLI command: `sourcecred export-graph`
This adds an `export-graph` command to the SourceCred CLI. It exports
the combined cred graphs for individual RepoIds, as was done for
[sourcecred/research#4].
Example usage:
```
$ node bin/sourcecred.js load sourcecred/mission
$ node bin/sourcecred.js export-graph sourcecred/mission >
/tmp/mission_graph.json
```
Test plan:
The new command is thoroughly unit tested.
[sourcecred/research#4]: https://github.com/sourcecred/research/pull/4
* Address review feedback by @wchargin
This commit makes several improvements to `repoIdRegistry`:
- Create `writeRegistry` and `getRegistry` methods to abstract over
needing to find the right file, populate an empty registry if its not
available, etc.
- Create `getEntry` for efficiently checking whether a RepoId is in the
registry
- Rename `addRepoId` to `addEntry` for consistency
- Add docstrings
The `load` command has been refactored to use the new methods.
Test plan: Unit tests added, and they pass. The `load` command is
already thoroughly tested, so regressions are very unlikely.
Part of ongoing work for #1020.
Test plan:
Added tests that mirror the edge filtering tests in `graph.test`
to check that `graph` and `pagerankGraph` return the same edges
with the given `EdgesOptions` parameter. Also added a sanity check
that a `weight` prop is returned from the iterator along with the edge.
Given the dependence on a helper function to test the edge
iterator's equality between graphs, I would suggest reviewers give
particular attention to that function:
`expectConsistentEdges()`
Summary:
@decentralion has used two emails to commit to Git: one exclusively
prior to 2018-05-21 and one exclusively after that date. This commit
adds a mailmap file to list their canonical email address. For more
information, see `man git-check-mailmap`:
<https://www.git-scm.com/docs/git-check-mailmap>
Test Plan:
See `git log --format=%ad --author=dl@` for dates of commits under the
old email, and `git log --format=%ad --author=dandelion@` for dates of
commits under the new email, to confirm the date ranges listed above.
Before this change:
$ git shortlog -nse | head -3
443 William Chargin <wchargin@gmail.com>
291 Dandelion Mané <decentralion@dandelion.io>
129 Dandelion Mané <dl@dandelion.io>
After this change:
$ git shortlog -nse | head -3
444 William Chargin <wchargin@gmail.com>
420 Dandelion Mané <decentralion@dandelion.io>
12 Brian Litwin <brian.n.litwin@gmail.com>
wchargin-branch: dandelion-mailmap
We switched from marking beginner-friendly issues "Contributions Welcome"
to "Good First Issue". See sourcecred/pm#15 for discussion.
Test Plan:
The new link works correctly on my local fork.
This commit adds a `totalOutWeight` method to `PagerankGraph`.
For any given node, `totalOutWeight` reports the total weight traveling
away from the node on edges (including the synthetic loop edge). Using
totalOutWeight makes it possible to normalize the weights to get the
actual markov transition probabilities.
Test plan: Unit tests verify the following properties:
- An error is thrown if the requested node does not exist.
- An error is thrown if the graph has been modified.
- The out weights are computed correctly in the standard case.
- The out weights are computed correctly in the case where there are no
weights (except the synthetic loop weight)
- The out weights are still computed correctly after
JSON-deserialization.
Inspired by a [suggestion] @decentralion made to improve #1105
This will enable `pagerankGraph` to throw an error when it is
called with invalid option parameters. Previously, to elicit
this error we had to access the iterator through `Array.from()`
or similar.
Test plan:
Yarn test passes.
Specifically, I removed the `Array.from()` wrapper around `pagerankGraph`
in the test that checks to see that `pagerankGraph` throws an error when
`nodes()` is passed invalid options.
[suggestion]: https://github.com/sourcecred/sourcecred/pull/1105#pullrequestreview-206496537
The motivation for this change is to make it easier
to access the selected Node's `name` prop for #576,
in which we plan to show a Card displaying summary
stats for the selected node. With only the `topLevelFilter`
available, it's trickier than it needs to be to find out
a node type's `name`.
Test Plan:
* Yarn test passes.
* Visual/Manual inspection of table doesn't surface any issues.
* Updated `it("filter defaults to defaultNodeFilter if available")`
to `it("selectedNodeType defaults to defaultNodeType if available")`.
* Verified that the above new test is failable in several ways by
mangling the tests to test for the wrong node type and mangling the
code to set the wrong node type.
* Since we factored out 'topLevelFilter' and 'defaultNodeFilter',
running `git grep -i topLevelFilter` and `git grep -i defaultNodeFilter`
turns up empty, just to make sure those terms aren't hanging
around to confuse anybody in the future.
* I don't think changing the `prop` parameter warrants any
additional tests, as the current tests verify that the prop
is passed in correctly.
This was at @decentralion's suggestion, following the Contributing
Guideline's Kent Beck quote of making the easy change to make the
change we were originally after (#576) easier. 🙌
* Highlight tableRows on :hover and :focus-within
Resolves#1041
The purpose of this commit is to make the pagerankTable easier
to read, as it's currently difficult to distinguish which score is
associated with which row because of the tight spacing of the
rows and the space between the score column and the row detail column.
@wchargin provided the implementation using `linearGradient()`
and `backgroundImage`s.
A side effect of highlighting the row on `focus-within` is that the rows
will become highlighted when the expand button is clicked, which we
decided was acceptable.
Test plan:
Yarn test passes.
To test the new highlight behavior, visual/manual inspection
passes.
Also added the Aphrodite className to the snapshot
tests. The combination of testing the className + inline style props
should make these tests sensitive to UI changes in the future.
Screenshots:
<img width="939" alt="screenshot 2019-02-17 15 46 34" src="https://user-images.githubusercontent.com/26695477/52918955-332f5280-32cb-11e9-87d3-887c8877116a.png">
<img width="931" alt="screenshot 2019-02-17 15 45 10" src="https://user-images.githubusercontent.com/26695477/52918953-2f9bcb80-32cb-11e9-9356-82c6dccab4ae.png">
* bump CI
Suggested by @decentralion in his review of #1090
Test plan:
yarn test passes. Also verified that the new test case is
failable, if you pass in the wrong array of nodes to `expect()` or
if you mangle the node filter code.
Continuing work on #1020.
Adding an optional parameter to `nodes()` which enables optional
node prefix filtering.
Test plan:
@decentralion suggested on Discord that the tests should verify:
1) the parameter was passed to `_graph` correctly
2) the augmentation logic was applied correctly
The tests I added are identical to the tests in `graph.test`, except
that they verify that the result of `pagerankGraph` matches that of
`graph`. On one hand, this creates a dependence on `graph`,
as these tests don't verify that the filter works correctly, only that
graph has applied the filter and returned the iterator.
However, my prevailing thought is that it isn't `pagerankGraph's` responsibility
to test the behavior of `graph`, and so testing the exact filter results
of `pagerankGraph` like we do in `graph.test` isn't the best strategy, and
testing that `pagerankGraph`'s results equal `graph`'s results is a better strategy.
The tests also check that a `score` is provided alongside each `node` in the iterator,
to minimally satisfy @decentralion's second spec.
yarn test passes.
* PagerankGraph: Add toJSON/fromJSON
This commit adds serialization logic to `PagerankGraph`. As with many
things in PagerankGraph, it's based on the corresponding logic in `Graph`.
Much like graph, it stores data associated with nodes and edges (in this
case, the scores and edge weights) in an ordered array rather than a
map, so as to avoid repetitiously serializing the node and edge
addresses.
Test plan: Unit tests added, and they should be sufficient. Also take a
look at the included snapshot.
Part of ongoing work for #1020.
Adds an equals method for the PagerankGraph. This is really quite
straightforward, the logic is based on the matching logic for
`Graph.equals`.
Tests added.
Test plan: The added tests are comprehensive, and they pass.
As discussed in #1004, we want to be able to package metadata with a
graph's nodes and edges. We can do this much more compactly if we store
the metadata as an array, ordered by the corresponding node/edge
address, rather than storing a map. The disadvantage is that clients
then need to manually sort the graph addresses to deserialize.
This commit adds public methods that allow a client to efficiently
retrieve the sorted addresses from the GraphJSON (where they are already
serialized). This behavior is tested. Note that we appropriately don't
allow clients to peek and directly depend on the exact representation of
GraphJSON, we just promise that sorted address retrieval is possible.
Test plan: Unit tests added (and I verified that breaking the sorting
will fail the test).
* Start work on the PagerankGraph
This commit begins work on the `PagerankGraph` class, as described in
[#1020]. As of this commit, the `PagerankGraph` has basic functionality
like retrieving nodes and edges, and running PageRank. However, it is
missing utility functionality like equality testing and serialization,
and doesn't yet have score decomposition logic.
This was mostly produced during a [live coding session]. Thanks to
@BrianLitwin, @anthrocypher, and @wchargin for participating.
Test plan:
The new code is thoroughly unit tested. Please review the test coverage,
and also the quality of the documentation.
[#1020]: https://github.com/sourcecred/sourcecred/issues/1020
[live coding session]: https://github.com/sourcecred/mission/issues/14
* Improvements from self-review
- Don't allow PRG around empty graph, as there's no way to make it
a valid probability distribution
* Add issue ref in TODOs
This commit modifies `markovChain.findStationaryDistribution` so that
in addition to returning the final distribution, it also reports the
final convergence delta.
This is motivated by the proposed API for the new PagerankGraph (see
[#1020]). Also, I think it makes a nice addition to the test code.
Note that this slightly changes the output from `findStationaryDistribution`,
because we now return the first distribution that is sufficiently converged,
rather than that distribution with one additional Markov action.
Test plan:
Unit tests are updated, and `yarn test` passes.
[#1020]: https://github.com/sourcecred/sourcecred/issues/1020
Thanks to @BrianLitwin for semi-pair-programming it
Thanks to @wchargin for extensive review feedback.
Pull #1080 added in a description field for edge types, but put in a
placeholder message for each actual description. This pull adds in
descriptions for each edge type.
Test plan: `yarn test` passes, and additionally
`git grep 'TODO: Add a description for this edge type'` returns no hits.
Reviewed by @BrianLitwin and @wchargin.
* Enable loading private git repositories
This commit enables loading private repositories, assuming that the user
has ssh-agent configured with keys to allow cloning the private
repository, and has provided a GitHub API token with permissions for the
repository in question.
I have not added automated testing. I don't think a cost-benefit
analysis favors adding such tests at this time:
- This code changes very infrequently, and so is unlikely to break
- If it does break, it will be pretty easy to catch and to fix
- the @sourcecred org is on a free plan, which doesn't allow private
repos, so setting up the test case is a bit of a pain
Test plan: `yarn test --full` passes, so I haven't broken existing Git
clone behavior. Locally, I am able to load private repositories.
* Remove unnecessary process import.
Fixes#1019.
Test plan: Loading the prototype works, as does clicking through to different prototype pages.
“Running `git grep -F '/prototypes/'` returns no results; before this commit, it yielded 2 results.”
This commit #811, allowing users to set the weights of node/edge types to 0.
The WeightSlider now sets the weight to 0 when its dragged to its minimum value.
The logic for converting between weights and sliders has also been made more robust,
and is more thoroughly tested.
In cases where we wanted to set the weight to 0 (e.g. backwards Reaction edges),
the default weight has been changed.
Test plan:
Loading the UI, check that the sliders still work as expected (dragging them changes the displayed weight, dragging to the far left sets weight to 0). Check that the weights are consumed as expected (setting weight for issues to 0 leads to no cred for issues). Check that the weights for backwards reaction edges now have 0 weight. `git grep "TODO(#811)"` returns no hits.
PR #1075 added a new EdgeType, and #1080 added a new field to EdgeTypes.
Both PRs merged and this broke the build.
This very trivial commit fixes the build breakage in a noncontroversial
way (copies the placeholder edge description used for every other edge
over).
Test plan: `yarn test` passes.
Dandelion Mané
Brian Litwin
Seth Benton
Ana Noemi
William Chargin
expravit
Ian Darrow
