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

feat: make model parameters editable #1499

Merged
merged 15 commits into from
Jul 19, 2023
Merged

feat: make model parameters editable #1499

merged 15 commits into from
Jul 19, 2023

Conversation

Jami159
Copy link
Contributor

@Jami159 Jami159 commented Jul 17, 2023

Description

@blanchco
Copy link
Contributor

Testing:

  1. Changing a parameter ID
    Propagates changes to default config, does not propagate to other configs (is this needed)?
    semantics.ode.rates.expression, semantics.ode.rates.expression_mathml, semantics.ode.parameters.id

  2. Change a parameter value
    Changes semantics.ode.parameters.value

  3. Changing a parameter ID which is a substring of another parameter ID
    Propagates changes to default config ✅

@YohannParis
Copy link
Member

  1. Changing a parameter ID
    Propagates changes to default config, does not propagate to other configs (is this needed)?
    semantics.ode.rates.expression, semantics.ode.rates.expression_mathml, semantics.ode.parameters.id

Yes this is needed to update all configs.

@Jami159 Jami159 requested review from dgauldie and dvince2 as code owners July 19, 2023 14:56
@blanchco
Copy link
Contributor

blanchco commented Jul 19, 2023

Qa round 2:

  • Successfully updates parameter ID for all model configurations:
    semantics.ode.rates.expression, semantics.ode.rates.expression_mathml, semantics.ode.parameters.id, metadata.timeseries

  • Successfully updates changes the parameter value - this does not propagate to other configurations as expected ✅

  • Naming a parameter ID that is the substring of another is does not affect other parameters ✅

  • naming a parameter ID the same as another parameter will cause issues. If this is done you will not be able to separate the ids again (perhaps we can just keep note of this) 😵‍💫

Screen.Recording.2023-07-19.at.11.23.10.AM.mov

@mwdchang mwdchang self-requested a review July 19, 2023 16:49
@Jami159 Jami159 merged commit df91cdf into main Jul 19, 2023
@Jami159 Jami159 deleted the make_model_tables_editable branch July 19, 2023 16:51
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.

4 participants