Skip to content
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

Share default TimelineCredParameters #1376

Merged
merged 1 commit into from Sep 12, 2019

Conversation

@decentralion
Copy link
Member

commented Sep 11, 2019

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.

@decentralion decentralion requested a review from Beanow Sep 11, 2019
@@ -56,3 +57,10 @@ export function paramsFromJSON(
weights: weightsFromJSON(p.weights),
};
}

export function defaultParams(weights?: Weights): TimelineCredParameters {

This comment has been minimized.

Copy link
@Beanow

Beanow Sep 12, 2019

Member

While I see your point about only weights being used for overrides. I feel like this approach (selective overrides, and a default parameters function) is somewhat unconventional. Most commonly I see optional arguments to a constructor or factory, which are then merged with the default values.

In some cases the defaults are also exposed, for example in the yaml package, because you may want to reuse your options object for multiple function calls. Though in this case, the TimelineCred class already acts as the way to store and reuse your options.

The bottom line is, I wouldn't make it the responsibility of the caller to know about defaultParams when constructing. Instead make it the constructor / factory's concern to fill in any parameters not provided. And not to be selective based on what's already used in the current code-base. As this convention will make exposing features in a library-style easier later on.

This comment has been minimized.

Copy link
@decentralion

decentralion Sep 12, 2019

Author Member

Good point. I refactored the usage so that all the API endpoints that accept parameters now accept an optional "partial parameters" object. Basically you can provide partial inputs like {weights: myCustomWeights, alpha: myCustomAlpha} and any missing fields get filled in automatically. PTAL.

@decentralion decentralion force-pushed the default-params branch from 12cd07a to 1913468 Sep 12, 2019
@decentralion decentralion changed the base branch from timeline-params-module to master Sep 12, 2019
@decentralion decentralion force-pushed the default-params branch from 1913468 to 72d6ca9 Sep 12, 2019
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.
@decentralion decentralion force-pushed the default-params branch from 72d6ca9 to 0c0f22e Sep 12, 2019
@Beanow
Beanow approved these changes Sep 12, 2019
@decentralion decentralion merged commit 0a0010f into master Sep 12, 2019
2 checks passed
2 checks passed
ci/circleci: publish-1 Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
@decentralion decentralion deleted the default-params branch Sep 12, 2019
@wchargin

This comment has been minimized.

Copy link
Member

commented Sep 15, 2019

This commit adds a new type, PartialTimelineCredParameters, which
is basically TimelineCredParameters with every field marked optional.

No need; use built-in $Shape<…>: see #1379.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.