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-4547 Use new batch API in preparation for multi-sensitivity #178

Merged
merged 11 commits into from
Sep 1, 2023

Conversation

EmmaLRussell
Copy link
Contributor

@EmmaLRussell EmmaLRussell commented Aug 30, 2023

This branch updates WODIN to use the new odin-js batch running API to support multi-sensitivity. The key difference are:

  • BatchPars no longer has a single name and values props, but instead supports multiple varying parameters, with a varying prop of type VaryingPar[], each of which specifies nameandvalue`
  • Sensitivity results are now of type OdinUserTypeSeriesSet where the x values are not numeric parameter values, but instead are UserTypes, each of which specifies values for all the varying parameters.

There are no functionality changes in this branch, everything should work as before.

There's a chain of branches which should be merged and updated as follows when this branch is merged:

@EmmaLRussell EmmaLRussell marked this pull request as ready for review August 31, 2023 08:49
Copy link
Collaborator

@M-Kusumgar M-Kusumgar left a comment

Choose a reason for hiding this comment

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

Code looks good and everything works well! I did discover a bug from my side actually (implementation of advanced settings) it doesnt update run required when changed on fit and sensitivity tabs, only run tab! created a ticket and will fix asap!

@@ -88,6 +89,9 @@ export default defineComponent({
const time = {
mode: "grid" as const, tStart: start, tEnd: end, nPoints: points
};
const varyingParamName = paramSettings.value.parameterToVary;
const varyingPar = pars.varying.filter((v: VaryingPar) => v.name === varyingParamName);
Copy link
Collaborator

Choose a reason for hiding this comment

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

oh btw, completely aside from the review, idk if you know this, i learnt it like a tiny bit ago, you can get the type of the store and you wont have to annotate these filters or maps or anything you get from the store with a type, you just have to do const store = useStore<AppState>() when you define store! Idk if theres any disadvantage to doing this let me know, I might start doing this in some places if not

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I always forget to do it!

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 works for me - let's get all the parts merged in! One branch pointer to update here when we do so

@@ -1,7 +1,7 @@
#!/usr/bin/env bash
set -ex

ODIN_API_BRANCH=main
ODIN_API_BRANCH=odin-js-0-1-2
Copy link
Member

Choose a reason for hiding this comment

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

This needs updating on merge

@EmmaLRussell EmmaLRussell merged commit 204997e into main Sep 1, 2023
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.

3 participants