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

mrc-4455 Support multiple varying parameters in batches #27

Merged
merged 10 commits into from
Sep 1, 2023
Merged

Conversation

EmmaLRussell
Copy link
Contributor

@EmmaLRussell EmmaLRussell commented Aug 17, 2023

Adds support for multiple varying parameters in the running of sensitivity. For consistency, running single parameter sensitivity is now done in the same way, with just a single varying parameter defined.

Key changes:

  • in BatchPars, the name and values props have been replaced by an array of VaryingPars (each of which has a parameter name and array of values).
  • expandVaryingParams method is used to generate all combinations of varying params values (as an array of UserTypes) - batchRun iterates through this array and computes for each of these combinations
  • BatchError has been replaced by RunStatus which is recorded for each combination of varying parameter values and includes success boolean. the varying parameter values and any error strings.
  • Two getters give useful access to the run statuses, the first returning errors only, and the second returning the successful parameter value combinations, which will correspond by index to the returned solutions list
  • the summary series sets which previously had the single varying parameter's values as its x values, now needs to specify the multiple varying parameter values, so there's now a UserTypeSeriesSet type where x is an array of UserTypes specifying these multiple values. (Maybe x isn't a sensible name for this any more!)

@codecov
Copy link

codecov bot commented Aug 18, 2023

Codecov Report

Patch coverage is 100.00% of modified lines.

Files Changed Coverage
src/solution.ts ø
src/versions.ts ø
src/batch.ts 100.00%

📢 Thoughts on this report? Let us know!.

@EmmaLRussell EmmaLRussell marked this pull request as ready for review August 18, 2023 15:18
@EmmaLRussell EmmaLRussell requested a review from richfitz August 18, 2023 15:18
This was referenced Aug 18, 2023
Copy link
Member

@richfitz richfitz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great, and less disruptive than expected. One suggestion about making the expansion part more resable.

@@ -67,14 +79,14 @@ export class Batch {
*/
public compute(): boolean {
if (this._pending.length > 0) {
const value = this._pending[0];
const values = this._pending[0];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh did we make this already non blocking? that was good of us

src/batch.ts Outdated
* represents a single combination of values for the varying parameters. We will combine these with the base
* pars for each run.
*/
private expandVaryingParams(varyingPars: VaryingPar[]): UserType[] {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this could be a free function rather than a method, and then would be easier to put a few unit tests on. I'd not be surprised if we use this elsewhere (in this project or adapt to another) so might be worthwhile.

@EmmaLRussell EmmaLRussell requested a review from richfitz August 22, 2023 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants