From 0c0f22e0f2eb33cb036068a9d1b90258ca80ed99 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dandelion=20Man=C3=A9?= Date: Wed, 11 Sep 2019 23:17:37 +0200 Subject: [PATCH] Share default TimelineCredParameters At present, every place in the codebase that needs TimelineCredParameters constructs them ad-hoc, meaning we don't have any shared defaults across different consumers. This commit adds a new type, `PartialTimelineCredParameters`, which is basically `TimelineCredParameters` with every field marked optional. Callers can then choose to override any fields where they want non-default values. A new internal `partialParams` function promotes these partial parameters to full parameters. All the public interfaces for using params (namely, `TimelineCred.compute` and `TimelineCred.reanalyze`) now accept optional partial params. If the params are not specified, default values are used; if partial params are provided, all the explicitly provided values are used, and unspecified values are initialized to default values. Test plan: A simple unit test was added to ensure that weights overrides work as intended. `git grep "intervalDecay: "` reveals that there are no other explicit parameter constructions in the codebase. All existing unit tests pass. --- src/analysis/timeline/params.js | 44 +++++++++++++++ src/analysis/timeline/params.test.js | 54 +++++++++++++++++-- src/analysis/timeline/timelineCred.js | 32 +++++++---- src/analysis/timeline/timelineCred.test.js | 7 +-- src/api/load.js | 12 ++--- src/api/load.test.js | 18 +++---- src/cli/load.js | 3 +- src/cli/load.test.js | 7 +-- .../TimelineCredViewInspectionTest.js | 4 +- src/explorer/TimelineExplorer.js | 6 +-- 10 files changed, 145 insertions(+), 42 deletions(-) diff --git a/src/analysis/timeline/params.js b/src/analysis/timeline/params.js index a567a9dac..5213a88b5 100644 --- a/src/analysis/timeline/params.js +++ b/src/analysis/timeline/params.js @@ -5,6 +5,7 @@ import { type WeightsJSON, toJSON as weightsToJSON, fromJSON as weightsFromJSON, + defaultWeights, } from "../weights"; /** @@ -31,6 +32,20 @@ export type TimelineCredParameters = {| +weights: Weights, |}; +export const DEFAULT_ALPHA = 0.05; +export const DEFAULT_INTERVAL_DECAY = 0.5; + +/** + * The PartialTimelineCredParameters are a version of TimelineCredParameters + * where every field has been marked optional, to make it convenient for API + * clients to override just the parameters they want to. + */ +export type PartialTimelineCredParameters = {| + +alpha?: number, + +intervalDecay?: number, + +weights?: Weights, +|}; + export type TimelineCredParametersJSON = {| +alpha: number, +intervalDecay: number, @@ -56,3 +71,32 @@ export function paramsFromJSON( weights: weightsFromJSON(p.weights), }; } + +/** + * Exports the default TimelineCredParameters. + * + * End consumers of SourceCred will not need to depend on this; it's + * provided for implementation of SourceCred's APIs. + */ +export function defaultParams(): TimelineCredParameters { + return { + alpha: DEFAULT_ALPHA, + intervalDecay: DEFAULT_INTERVAL_DECAY, + weights: defaultWeights(), + }; +} + +/** + * Promote PartialTimelineCredParameters to TimelineCredParameters. + * + * This takes PartialTimelineCredParameters and mixes them with the + * default parameters to provide a full TimelineCredParameters. + * + * End consumers of SourceCred will not need to depend on this; it's + * provided for implementation of SourceCred's APIs. + */ +export function partialParams( + partial: PartialTimelineCredParameters +): TimelineCredParameters { + return {...defaultParams(), ...partial}; +} diff --git a/src/analysis/timeline/params.test.js b/src/analysis/timeline/params.test.js index bff9a2791..64665820d 100644 --- a/src/analysis/timeline/params.test.js +++ b/src/analysis/timeline/params.test.js @@ -1,19 +1,67 @@ // @flow -import {paramsToJSON, paramsFromJSON} from "./params"; +import { + paramsToJSON, + paramsFromJSON, + defaultParams, + partialParams, + type TimelineCredParameters, + DEFAULT_ALPHA, + DEFAULT_INTERVAL_DECAY, +} from "./params"; import {defaultWeights} from "../weights"; import {NodeAddress} from "../../core/graph"; describe("analysis/timeline/params", () => { - it("JSON round trip", () => { + const customWeights = () => { const weights = defaultWeights(); // Ensure it works with non-default weights weights.nodeManualWeights.set(NodeAddress.empty, 33); - const p = {alpha: 0.5, intervalDecay: 0.5, weights}; + 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); const j_ = paramsToJSON(p_); expect(j).toEqual(j_); expect(p).toEqual(p_); }); + it("defaultParams", () => { + const expected: TimelineCredParameters = { + alpha: DEFAULT_ALPHA, + intervalDecay: DEFAULT_INTERVAL_DECAY, + weights: defaultWeights(), + }; + expect(defaultParams()).toEqual(expected); + }); + describe("partialParams", () => { + it("uses default values if no overrides provided", () => { + const params = partialParams({}); + expect(params).toEqual(defaultParams()); + }); + it("accepts an alpha override", () => { + const params = partialParams({alpha: 0.99}); + expect(params.weights).toEqual(defaultWeights()); + 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(defaultWeights()); + expect(params.alpha).toEqual(DEFAULT_ALPHA); + expect(params.intervalDecay).toEqual(0.1); + }); + }); }); diff --git a/src/analysis/timeline/timelineCred.js b/src/analysis/timeline/timelineCred.js index 42224d502..537333c11 100644 --- a/src/analysis/timeline/timelineCred.js +++ b/src/analysis/timeline/timelineCred.js @@ -21,10 +21,12 @@ import { paramsToJSON, paramsFromJSON, type TimelineCredParametersJSON, + type PartialTimelineCredParameters, + partialParams, + defaultParams, } from "./params"; export type {Interval} from "./interval"; -export type {TimelineCredParameters} from "./params"; /** * A Graph Node wrapped with cred information. @@ -85,8 +87,14 @@ export class TimelineCred { * * This returns a new TimelineCred; it does not modify the existing one. */ - async reanalyze(newParams: TimelineCredParameters): Promise { - return await TimelineCred.compute(this._graph, newParams, this._plugins); + async reanalyze( + newParams: PartialTimelineCredParameters + ): Promise { + return await TimelineCred.compute({ + graph: this._graph, + params: newParams, + plugins: this._plugins, + }); } /** @@ -217,11 +225,13 @@ export class TimelineCred { return new TimelineCred(graph, intervalsJSON, cred, params, pluginsJSON); } - static async compute( + static async compute(opts: {| graph: Graph, - params: TimelineCredParameters, - plugins: $ReadOnlyArray - ): Promise { + params?: PartialTimelineCredParameters, + plugins: $ReadOnlyArray, + |}): Promise { + const {graph, params, plugins} = opts; + const fullParams = params == null ? defaultParams() : partialParams(params); const nodeOrder = Array.from(graph.nodes()).map((x) => x.address); const types = combineTypes(plugins); const userTypes = [].concat(...plugins.map((x) => x.userTypes)); @@ -229,9 +239,9 @@ export class TimelineCred { const distribution = await timelinePagerank( graph, types, - params.weights, - params.intervalDecay, - params.alpha + fullParams.weights, + fullParams.intervalDecay, + fullParams.alpha ); const cred = distributionToCred( distribution, @@ -249,7 +259,7 @@ export class TimelineCred { graph, intervals, addressToCred, - params, + fullParams, plugins ); return preliminaryCred.reduceSize({ diff --git a/src/analysis/timeline/timelineCred.test.js b/src/analysis/timeline/timelineCred.test.js index 6339751d8..757dd2450 100644 --- a/src/analysis/timeline/timelineCred.test.js +++ b/src/analysis/timeline/timelineCred.test.js @@ -11,8 +11,8 @@ import { EdgeAddress, } from "../../core/graph"; import {TimelineCred} from "./timelineCred"; +import {defaultParams} from "./params"; import {type PluginDeclaration} from "../pluginDeclaration"; -import {defaultWeights} from "../weights"; import {type NodeType} from "../types"; describe("src/analysis/timeline/timelineCred", () => { @@ -84,8 +84,9 @@ describe("src/analysis/timeline/timelineCred", () => { const scores = intervals.map((_) => i); addressToCred.set(address, scores); } - const params = {alpha: 0.05, intervalDecay: 0.5, weights: defaultWeights()}; - return new TimelineCred(graph, intervals, addressToCred, params, [plugin]); + return new TimelineCred(graph, intervals, addressToCred, defaultParams(), [ + plugin, + ]); } it("JSON serialization works", () => { diff --git a/src/api/load.js b/src/api/load.js index b321702dd..18e1d08db 100644 --- a/src/api/load.js +++ b/src/api/load.js @@ -6,10 +6,9 @@ import path from "path"; import {TaskReporter} from "../util/taskReporter"; import {Graph} from "../core/graph"; import {loadGraph} from "../plugins/github/loadGraph"; -import { - type TimelineCredParameters, - TimelineCred, -} from "../analysis/timeline/timelineCred"; +import {TimelineCred} from "../analysis/timeline/timelineCred"; +import {defaultParams, partialParams} from "../analysis/timeline/params"; +import {type PartialTimelineCredParameters} from "../analysis/timeline/params"; import {type Project} from "../core/project"; import {setupProjectDirectory} from "../core/project_io"; @@ -19,7 +18,7 @@ import * as NullUtil from "../util/null"; export type LoadOptions = {| +project: Project, - +params: TimelineCredParameters, + +params: ?PartialTimelineCredParameters, +plugins: $ReadOnlyArray, +sourcecredDirectory: string, +githubToken: string | null, @@ -45,6 +44,7 @@ export async function load( taskReporter: TaskReporter ): Promise { const {project, params, plugins, sourcecredDirectory, githubToken} = options; + const fullParams = params == null ? defaultParams() : partialParams(params); const loadTask = `load-${options.project.id}`; taskReporter.start(loadTask); const cacheDirectory = path.join(sourcecredDirectory, "cache"); @@ -101,7 +101,7 @@ export async function load( await fs.writeFile(graphFile, JSON.stringify(graph.toJSON())); taskReporter.start("compute-cred"); - const cred = await TimelineCred.compute(graph, params, plugins); + const cred = await TimelineCred.compute({graph, params: fullParams, plugins}); const credJSON = cred.toJSON(); const credFile = path.join(projectDirectory, "cred.json"); await fs.writeFile(credFile, JSON.stringify(credJSON)); diff --git a/src/api/load.test.js b/src/api/load.test.js index 6da821722..92a2b53a2 100644 --- a/src/api/load.test.js +++ b/src/api/load.test.js @@ -19,6 +19,10 @@ import {NodeAddress, Graph} from "../core/graph"; import {node} from "../core/graphTestUtil"; import {TestTaskReporter} from "../util/taskReporter"; import {load, type LoadOptions} from "./load"; +import { + type PartialTimelineCredParameters, + partialParams, +} from "../analysis/timeline/params"; type JestMockFn = $Call; jest.mock("../plugins/github/loadGraph", () => ({ @@ -70,7 +74,7 @@ describe("api/load", () => { // Tweaks the weights so that we can ensure we aren't overriding with default weights weights.nodeManualWeights.set(NodeAddress.empty, 33); // Deep freeze will freeze the weights, too - const params = deepFreeze({alpha: 0.05, intervalDecay: 0.5, weights}); + const params: PartialTimelineCredParameters = {weights}; const plugins = deepFreeze([]); const example = () => { const sourcecredDirectory = tmp.dirSync().name; @@ -139,14 +143,10 @@ describe("api/load", () => { it("calls TimelineCred.compute with the right graph and options", async () => { const {options, taskReporter} = example(); await load(options, taskReporter); - expect(timelineCredCompute).toHaveBeenCalledWith( - expect.anything(), - params, - plugins - ); - expect(timelineCredCompute.mock.calls[0][0].equals(combinedGraph())).toBe( - true - ); + const args = timelineCredCompute.mock.calls[0][0]; + expect(args.graph.equals(combinedGraph())).toBe(true); + expect(args.params).toEqual(partialParams(params)); + expect(args.plugins).toEqual(plugins); }); it("saves the resultant cred.json to disk", async () => { diff --git a/src/cli/load.js b/src/cli/load.js index 8e0f9b25e..83455ea05 100644 --- a/src/cli/load.js +++ b/src/cli/load.js @@ -9,6 +9,7 @@ import {defaultWeights, fromJSON as weightsFromJSON} from "../analysis/weights"; import {load} from "../api/load"; import {specToProject} from "../plugins/github/specToProject"; import fs from "fs-extra"; +import {partialParams} from "../analysis/timeline/params"; import {DEFAULT_PLUGINS} from "./defaultPlugins"; function usage(print: (string) => void): void { @@ -105,7 +106,7 @@ const loadCommand: Command = async (args, std) => { const projects = await Promise.all( projectSpecs.map((s) => specToProject(s, githubToken)) ); - const params = {alpha: 0.05, intervalDecay: 0.5, weights}; + const params = partialParams({weights}); const plugins = DEFAULT_PLUGINS; const optionses = projects.map((project) => ({ project, diff --git a/src/cli/load.test.js b/src/cli/load.test.js index 69d3e4733..35660e4f6 100644 --- a/src/cli/load.test.js +++ b/src/cli/load.test.js @@ -11,6 +11,7 @@ import type {LoadOptions} from "../api/load"; import {defaultWeights, toJSON as weightsToJSON} from "../analysis/weights"; import * as Common from "./common"; import {DEFAULT_PLUGINS} from "./defaultPlugins"; +import {defaultParams, partialParams} from "../analysis/timeline/params"; import {makeRepoId, stringToRepoId} from "../core/repoId"; @@ -78,7 +79,7 @@ describe("cli/load", () => { discourseServer: null, }, plugins: DEFAULT_PLUGINS, - params: {alpha: 0.05, intervalDecay: 0.5, weights: defaultWeights()}, + params: defaultParams(), sourcecredDirectory: Common.sourcecredDirectory(), githubToken: fakeGithubToken, discourseKey: fakeDiscourseKey, @@ -102,7 +103,7 @@ describe("cli/load", () => { repoIds: [stringToRepoId(projectId)], discourseServer: null, }, - params: {alpha: 0.05, intervalDecay: 0.5, weights: defaultWeights()}, + params: defaultParams(), plugins: DEFAULT_PLUGINS, sourcecredDirectory: Common.sourcecredDirectory(), githubToken: fakeGithubToken, @@ -140,7 +141,7 @@ describe("cli/load", () => { repoIds: [makeRepoId("foo", "bar")], discourseServer: null, }, - params: {alpha: 0.05, intervalDecay: 0.5, weights}, + params: partialParams({weights}), plugins: DEFAULT_PLUGINS, sourcecredDirectory: Common.sourcecredDirectory(), githubToken: fakeGithubToken, diff --git a/src/explorer/TimelineCredViewInspectionTest.js b/src/explorer/TimelineCredViewInspectionTest.js index 6b17d62e0..7765dab2c 100644 --- a/src/explorer/TimelineCredViewInspectionTest.js +++ b/src/explorer/TimelineCredViewInspectionTest.js @@ -6,7 +6,7 @@ import type {Assets} from "../webutil/assets"; import {TimelineCredView} from "./TimelineCredView"; import {Graph, NodeAddress} from "../core/graph"; import {type Interval, TimelineCred} from "../analysis/timeline/timelineCred"; -import {defaultWeights} from "../analysis/weights"; +import {defaultParams} from "../analysis/timeline/params"; export default class TimelineCredViewInspectiontest extends React.Component<{| +assets: Assets, @@ -46,7 +46,7 @@ export default class TimelineCredViewInspectiontest extends React.Component<{| const scores = intervals.map((_unuesd, i) => generator(i)); addressToCred.set(address, scores); } - const params = {alpha: 0.05, intervalDecay: 0.5, weights: defaultWeights()}; + const params = defaultParams(); return new TimelineCred(graph, intervals, addressToCred, params, []); } diff --git a/src/explorer/TimelineExplorer.js b/src/explorer/TimelineExplorer.js index 755ead1c8..0805e3fc6 100644 --- a/src/explorer/TimelineExplorer.js +++ b/src/explorer/TimelineExplorer.js @@ -4,10 +4,8 @@ import React from "react"; import deepEqual from "lodash.isequal"; import {type Weights, copy as weightsCopy} from "../analysis/weights"; import {type NodeAddressT} from "../core/graph"; -import { - TimelineCred, - type TimelineCredParameters, -} from "../analysis/timeline/timelineCred"; +import {TimelineCred} from "../analysis/timeline/timelineCred"; +import {type TimelineCredParameters} from "../analysis/timeline/params"; import {TimelineCredView} from "./TimelineCredView"; import Link from "../webutil/Link"; import {WeightConfig} from "./weights/WeightConfig";