Move weights out of TimelineCredParams (#1607)

This commit moves weights out of the "parameters" to TimelineCred. This
makes sense, because the Weights are now passed to TimelineCred via the
included WeightedGraph. As such, we now have the `api/load` options
include explicit Weights that are used as overrides, rather than having
them be included in the TimelineCred parameters.

Test plan: I've manually tested this commit by:
- Changing weights in the explorer, and verifying that the `recalculate
cred` button activates as expected, and the new weights are used
correctly in the resultant distribution.
- Verifying that downloading weights form the UI still works.
- Verified that uploading weights to the UI still works.
- Verifying that passing command-line weights files still works.

Also, `yarn test` passes.

src/analysis/timeline/params.js

@@ -1,8 +1,5 @@
 // @flow
 
-import {type Weights as WeightsT, type WeightsJSON} from "../../core/weights";
-import * as Weights from "../../core/weights";
-
 /**
  * Parameters for computing TimelineCred
  *
@@ -20,11 +17,6 @@ export type TimelineCredParameters = {|
   // decay is 0, then weights go to zero the first week after their node/edge
   // was created. Should be between 0 and 1.
   +intervalDecay: number,
-  // The weights. This determines how much cred is assigned based on different
-  // node types, how cred flows across various edge types, and can specify
-  // manual weights directly on individual nodes. See the docs in
-  // `analysis/weights` for details.
-  +weights: WeightsT,
 |};
 
 export const DEFAULT_ALPHA = 0.2;
@@ -33,7 +25,6 @@ export const DEFAULT_INTERVAL_DECAY = 0.5;
 export type TimelineCredParametersJSON = {|
   +alpha: number,
   +intervalDecay: number,
-  +weights: WeightsJSON,
 |};
 
 export function paramsToJSON(
@@ -42,7 +33,6 @@ export function paramsToJSON(
   return {
     alpha: p.alpha,
     intervalDecay: p.intervalDecay,
-    weights: Weights.toJSON(p.weights),
   };
 }
 
@@ -52,7 +42,6 @@ export function paramsFromJSON(
   return {
     alpha: p.alpha,
     intervalDecay: p.intervalDecay,
-    weights: Weights.fromJSON(p.weights),
   };
 }
 
@@ -66,7 +55,6 @@ export function defaultParams(): TimelineCredParameters {
   return {
     alpha: DEFAULT_ALPHA,
     intervalDecay: DEFAULT_INTERVAL_DECAY,
-    weights: Weights.empty(),
   };
 }
 

src/analysis/timeline/params.test.js

@@ -9,21 +9,12 @@ import {
   DEFAULT_ALPHA,
   DEFAULT_INTERVAL_DECAY,
 } from "./params";
-import * as Weights from "../../core/weights";
-import {NodeAddress} from "../../core/graph";
 
 describe("analysis/timeline/params", () => {
-  const customWeights = () => {
-    const weights = Weights.empty();
-    // Ensure it works with non-default weights
-    weights.nodeWeights.set(NodeAddress.empty, 33);
-    return weights;
-  };
   it("JSON round trip", () => {
     const p: TimelineCredParameters = {
       alpha: 0.1337,
       intervalDecay: 0.31337,
-      weights: customWeights(),
     };
     const j = paramsToJSON(p);
     const p_ = paramsFromJSON(j);
@@ -35,7 +26,6 @@ describe("analysis/timeline/params", () => {
     const expected: TimelineCredParameters = {
       alpha: DEFAULT_ALPHA,
       intervalDecay: DEFAULT_INTERVAL_DECAY,
-      weights: Weights.empty(),
     };
     expect(defaultParams()).toEqual(expected);
   });
@@ -46,20 +36,11 @@ describe("analysis/timeline/params", () => {
     });
     it("accepts an alpha override", () => {
       const params = partialParams({alpha: 0.99});
-      expect(params.weights).toEqual(Weights.empty());
       expect(params.alpha).toEqual(0.99);
       expect(params.intervalDecay).toEqual(DEFAULT_INTERVAL_DECAY);
     });
-    it("accepts weights override", () => {
-      const weights = customWeights();
-      const params = partialParams({weights});
-      expect(params.weights).toEqual(weights);
-      expect(params.alpha).toEqual(DEFAULT_ALPHA);
-      expect(params.intervalDecay).toEqual(DEFAULT_INTERVAL_DECAY);
-    });
     it("accepts intervalDecay override", () => {
       const params = partialParams({intervalDecay: 0.1});
-      expect(params.weights).toEqual(Weights.empty());
       expect(params.alpha).toEqual(DEFAULT_ALPHA);
       expect(params.intervalDecay).toEqual(0.1);
     });

src/analysis/timeline/timelineCred.js

@@ -11,6 +11,7 @@ import {distributionToCred} from "./distributionToCred";
 import {type PluginDeclaration, combineTypes} from "../pluginDeclaration";
 import {type NodeAddressT, NodeAddress, type Node} from "../../core/graph";
 import * as WeightedGraph from "../../core/weightedGraph";
+import {type Weights as WeightsT} from "../../core/weights";
 import type {
   WeightedGraph as WeightedGraphT,
   WeightedGraphJSON,
@@ -86,10 +87,14 @@ export class TimelineCred {
    * This returns a new TimelineCred; it does not modify the existing one.
    */
   async reanalyze(
+    newWeights: WeightsT,
     newParams: $Shape<TimelineCredParameters>
   ): Promise<TimelineCred> {
     return await TimelineCred.compute({
-      weightedGraph: this._weightedGraph,
+      weightedGraph: WeightedGraph.overrideWeights(
+        this._weightedGraph,
+        newWeights
+      ),
       params: newParams,
       plugins: this._plugins,
     });
@@ -241,7 +246,7 @@ export class TimelineCred {
     plugins: $ReadOnlyArray<PluginDeclaration>,
   |}): Promise<TimelineCred> {
     const {weightedGraph, params, plugins} = opts;
-    const {graph} = weightedGraph;
+    const {graph, weights} = weightedGraph;
     const fullParams = params == null ? defaultParams() : partialParams(params);
     const nodeOrder = Array.from(graph.nodes()).map((x) => x.address);
     const types = combineTypes(plugins);
@@ -250,7 +255,7 @@ export class TimelineCred {
     const distribution = await timelinePagerank(
       graph,
       types,
-      fullParams.weights,
+      weights,
       fullParams.intervalDecay,
       fullParams.alpha
     );

src/api/load.js

@@ -16,11 +16,13 @@ import {type PluginDeclaration} from "../analysis/pluginDeclaration";
 import * as Discourse from "../plugins/discourse/loadWeightedGraph";
 import * as Github from "../plugins/github/loadWeightedGraph";
 import * as WeightedGraph from "../core/weightedGraph";
+import {type Weights as WeightsT} from "../core/weights";
 import {loadWeightedGraph} from "./loadWeightedGraph";
 
 export type LoadOptions = {|
   +project: Project,
   +params: ?$Shape<TimelineCredParameters>,
+  +weightsOverrides: WeightsT,
   +plugins: $ReadOnlyArray<PluginDeclaration>,
   +sourcecredDirectory: string,
   +githubToken: ?GithubToken,
@@ -44,7 +46,14 @@ export async function load(
   options: LoadOptions,
   taskReporter: TaskReporter
 ): Promise<void> {
-  const {project, params, plugins, sourcecredDirectory, githubToken} = options;
+  const {
+    project,
+    params,
+    plugins,
+    sourcecredDirectory,
+    githubToken,
+    weightsOverrides,
+  } = options;
   const {identities, discourseServer} = project;
   const fullParams = params == null ? defaultParams() : partialParams(params);
   const loadTask = `load-${options.project.id}`;
@@ -82,7 +91,7 @@ export async function load(
       discourseOptions,
       githubOptions,
       identitySpec,
-      weightsOverrides: fullParams.weights,
+      weightsOverrides,
     },
     taskReporter
   );

src/api/load.test.js

@@ -75,8 +75,8 @@ describe("api/load", () => {
     discourseServer: {serverUrl: discourseServerUrl},
   });
   deepFreeze(project);
-  const weights = Weights.empty();
-  const params: $Shape<TimelineCredParameters> = {weights};
+  const weightsOverrides = Weights.empty();
+  const params: $Shape<TimelineCredParameters> = {};
   const plugins = deepFreeze([]);
   const example = () => {
     const sourcecredDirectory = tmp.dirSync().name;
@@ -87,6 +87,7 @@ describe("api/load", () => {
       params,
       plugins,
       project,
+      weightsOverrides,
     };
     return {options, taskReporter, sourcecredDirectory};
   };

src/cli/discourse.js

@@ -97,7 +97,8 @@ const command: Command = async (args, std) => {
   await load(
     {
       project,
-      params: {weights},
+      params: null,
+      weightsOverrides: weights,
       plugins,
       sourcecredDirectory: Common.sourcecredDirectory(),
       githubToken: null,

src/cli/load.js

@@ -10,11 +10,11 @@ import {projectFromJSON} from "../core/project";
 import {load} from "../api/load";
 import {specToProject} from "../plugins/github/specToProject";
 import fs from "fs-extra";
-import {partialParams} from "../analysis/timeline/params";
 import {type PluginDeclaration} from "../analysis/pluginDeclaration";
 import {declaration as discourseDeclaration} from "../plugins/discourse/declaration";
 import {declaration as githubDeclaration} from "../plugins/github/declaration";
 import {declaration as identityDeclaration} from "../plugins/identity/declaration";
+import {defaultParams} from "../analysis/timeline/params";
 
 function usage(print: (string) => void): void {
   print(
@@ -122,7 +122,6 @@ const loadCommand: Command = async (args, std) => {
   const specProjects = await Promise.all(
     projectSpecs.map((s) => specToProject(s, githubToken))
   );
-  const params = partialParams({weights});
   const manualProjects = await Promise.all(projectPaths.map(loadProject));
   const projects = specProjects.concat(manualProjects);
   const optionses = projects.map((project) => {
@@ -138,7 +137,8 @@ const loadCommand: Command = async (args, std) => {
     }
     return {
       project,
-      params,
+      params: defaultParams(),
+      weightsOverrides: weights,
       plugins,
       sourcecredDirectory: Common.sourcecredDirectory(),
       githubToken,

src/cli/load.test.js

@@ -10,11 +10,11 @@ import loadCommand, {help} from "./load";
 import type {LoadOptions} from "../api/load";
 import * as Weights from "../core/weights";
 import * as Common from "./common";
-import {defaultParams, partialParams} from "../analysis/timeline/params";
 import {declaration as githubDeclaration} from "../plugins/github/declaration";
 import {createProject} from "../core/project";
 import {makeRepoId, stringToRepoId} from "../plugins/github/repoId";
 import {validateToken} from "../plugins/github/token";
+import {defaultParams} from "../analysis/timeline/params";
 
 jest.mock("../api/load", () => ({load: jest.fn()}));
 type JestMockFn = $Call<typeof jest.fn>;
@@ -77,6 +77,7 @@ describe("cli/load", () => {
           repoIds: [makeRepoId("foo", "bar")],
         }),
         params: defaultParams(),
+        weightsOverrides: Weights.empty(),
         plugins: [githubDeclaration],
         sourcecredDirectory: Common.sourcecredDirectory(),
         githubToken: exampleGithubToken,
@@ -100,6 +101,7 @@ describe("cli/load", () => {
           repoIds: [stringToRepoId(projectId)],
         }),
         params: defaultParams(),
+        weightsOverrides: Weights.empty(),
         plugins: [githubDeclaration],
         sourcecredDirectory: Common.sourcecredDirectory(),
         githubToken: exampleGithubToken,
@@ -135,7 +137,8 @@ describe("cli/load", () => {
           id: "foo/bar",
           repoIds: [makeRepoId("foo", "bar")],
         }),
-        params: partialParams({weights}),
+        params: defaultParams(),
+        weightsOverrides: weights,
         plugins: [githubDeclaration],
         sourcecredDirectory: Common.sourcecredDirectory(),
         githubToken: exampleGithubToken,

src/explorer/TimelineExplorer.js

@@ -2,7 +2,8 @@
 
 import React from "react";
 import deepEqual from "lodash.isequal";
-import {type Weights, copy as weightsCopy} from "../core/weights";
+import * as Weights from "../core/weights";
+import {type Weights as WeightsT} from "../core/weights";
 import {type NodeAddressT} from "../core/graph";
 import {TimelineCred} from "../analysis/timeline/timelineCred";
 import {type TimelineCredParameters} from "../analysis/timeline/params";
@@ -21,7 +22,7 @@ export type Props = {
 
 export type State = {
   timelineCred: TimelineCred,
-  weights: Weights,
+  weights: WeightsT,
   alpha: number,
   intervalDecay: number,
   loading: boolean,
@@ -41,7 +42,7 @@ export class TimelineExplorer extends React.Component<Props, State> {
   constructor(props: Props) {
     super(props);
     const timelineCred = props.initialTimelineCred;
-    const {alpha, intervalDecay, weights} = timelineCred.params();
+    const {alpha, intervalDecay} = timelineCred.params();
     this.state = {
       timelineCred,
       alpha,
@@ -49,7 +50,7 @@ export class TimelineExplorer extends React.Component<Props, State> {
       // Set the weights to a copy, to ensure we don't mutate the weights in the
       // initialTimelineCred. This enables e.g. disabling the analyze button
       // when the parameters are unchanged.
-      weights: weightsCopy(weights),
+      weights: Weights.copy(timelineCred.weightedGraph().weights),
       loading: false,
       showWeightConfig: false,
       selectedNodeTypePrefix: null,
@@ -57,15 +58,22 @@ export class TimelineExplorer extends React.Component<Props, State> {
   }
 
   params(): TimelineCredParameters {
-    const {alpha, intervalDecay, weights} = this.state;
+    const {alpha, intervalDecay} = this.state;
+    return {alpha, intervalDecay};
+  }
+
+  weights(): WeightsT {
     // Set the weights to a copy, to ensure that the weights we pass into e.g.
     // analyzeCred are a distinct reference from the one we keep in our state.
-    return {alpha, intervalDecay, weights: weightsCopy(weights)};
+    return Weights.copy(this.state.weights);
   }
 
   async analyzeCred() {
     this.setState({loading: true});
-    const timelineCred = await this.state.timelineCred.reanalyze(this.params());
+    const timelineCred = await this.state.timelineCred.reanalyze(
+      this.weights(),
+      this.params()
+    );
     this.setState({timelineCred, loading: false});
   }
 
@@ -74,7 +82,7 @@ export class TimelineExplorer extends React.Component<Props, State> {
     const weightFileManager = (
       <WeightsFileManager
         weights={this.state.weights}
-        onWeightsChange={(weights: Weights) => {
+        onWeightsChange={(weights: WeightsT) => {
           this.setState({weights});
         }}
       />
@@ -111,10 +119,12 @@ export class TimelineExplorer extends React.Component<Props, State> {
         }}
       />
     );
-    const paramsUpToDate = deepEqual(
-      this.params(),
-      this.state.timelineCred.params()
-    );
+    const paramsUpToDate =
+      deepEqual(this.params(), this.state.timelineCred.params()) &&
+      deepEqual(
+        this.weights(),
+        this.state.timelineCred.weightedGraph().weights
+      );
     const analyzeButton = (
       <button
         disabled={this.state.loading || paramsUpToDate}