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

add the reformulated version of Fiedler_BMCSystBiol2016 #258

Merged
merged 19 commits into from
Jan 4, 2025

Conversation

m-philipps
Copy link
Collaborator

  • closes issue Add reformulated Fiedler_BMCSystBiol2016 #252
  • add reformulated version of the existing Fiedler_BMCSystBiol2016 model, taken from the same publication, that removes non-identifiabilities
  • add a datasetId column for the existing Fiedler_BMCSystBiol2016 model

@FFroehlich
Copy link
Collaborator

would assume that all of the data is the same – why not just add separate model/parameters/yaml file to the base directory and reuse everything else?

@m-philipps
Copy link
Collaborator Author

The data is the same, the observable Ids changed (e.g. pErk -> pErk_rel) and that is propagated through the noise and observable parameters. Only the experimental condition files are the same.

I could rename the observables and associated parameters but I have a minor preference for keeping the new names to distinguish the interpretation of the observables in the old and reformulated model.

@m-philipps m-philipps requested a review from dilpath January 4, 2025 15:26
Copy link
Collaborator

@dilpath dilpath left a comment

Choose a reason for hiding this comment

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

Looks good. Since this is similar to submitting a new problem, you could add the checklist to this PR and indicate what is done/not done (of course, not everything has to be done since this is not the "main" problem -- simply leave those unchecked as a matter of record).

@m-philipps
Copy link
Collaborator Author

  • The PEtab problem is based on a model that is peer-reviewed and published
  • The problem ID is in the format {LAST_NAME_OF_FIRST_AUTHOR}_{ABBREVIATED_JOURNAL_NAME}{YEAR_OF_PUBLICATION}
    $\to$ No, Fiedler_BMCSystBiol206 already exists, ID is Fiedler_BMCSystBiol206_rel
  • The problem ID is in the pull request title
  • There is a GitHub issue for this problem
    • The problem ID is in the issue title
    • A brief model description (one or two sentences)
    • A brief data description (one or two sentences)
    • The issue and PR are linked to each other
    • Differences between the implementation and the original publication are described
    • Experience of fitting / uncertainty analysis (e.g. optimizer used, hyperparameters, reproducibility of best fit)
    • Source of nominal parameters (e.g.: taken from the original publication, or from your own fitting)
  • The SBML file
    • Annotation with reference to the original publication (example)
    • The model ID and model name attributes in the SBML model file match the problem name (example)
  • PEtab files
    • A "simulated data" measurement table is included, using the nominal parameters
    • A visualization table is included, that can be used with the simulated data to reproduce figures from the original publication
    • The PEtab problem is valid (check with e.g. petablint -vy problem.yaml)
  • The PEtab problem author(s) are assigned to the GitHub issue
  • The README has been updated with bmp-create-overview --update (requires pip install -e src/python/bmp from the repository root)
    • The new PEtab problem row in the generated table has the correct reference (and other entries)

@dilpath
Copy link
Collaborator

dilpath commented Jan 4, 2025

  • Annotation with reference to the original publication (example)

I think this is missing, but also fine to uncheck this box instead.

@m-philipps m-philipps merged commit 3ab5c5f into master Jan 4, 2025
6 checks passed
@m-philipps m-philipps deleted the fiedler_reparam branch January 4, 2025 21:49
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