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 Lang_PLOSComputBiol2024 #202

Merged

Conversation

paulflang
Copy link
Contributor

Is the readme up to date that you still welcome additional benchmark models? If so, I'd like to contribute this.

A few points to mention on this PR:

  • The following works locally:
>>> import benchmark_models_petab as models
>>> problem = models.get_problem('Lang_PLOSComputBiol2024')
>>> assert problem.measurement_df is not None
  • petablint complains about using observables in the noiseFormula. saCeSS (and I believe parPE) did not have a problem with that. This can be changed but will be less concise.
Missing parameter(s) in the model or the parameters table: {'obs_cycA__cyt_median', 'obs_p21__cyt_median', 'obs_cycE__nuc_median', 'obs_pRB__cyt_median', 'obs_p21__nuc_median', 'obs_cycA__nuc_median', 'obs_E2F1__cyt_median', 'obs_Skp2__nuc_median', 'obs_Skp2__cyt_median', 'obs_cycB1__cyt_median', 'obs_p27__cyt_median', 'obs_cycB1__nuc_median', 'obs_E2F1__nuc_median', 'obs_cycE__cyt_median', 'obs_p27__nuc_median', 'obs_pRB__nuc_median'}   
Not OK
  • The other models contain observable and noise parameter overrides in the measurement table, which is not needed in my case, but can be changed, too, of course.

  • The SBML file contains the optimized parameters. I've never tried an optimization with this file, but I don't see a reason why it shouldn't work 🤞

  • More generally, please lmk what else I need to do.

@m-philipps m-philipps self-assigned this Apr 17, 2024
@dweindl
Copy link
Member

dweindl commented Apr 21, 2024

Thanks Paul. Sorry for the late response. Contributions are still very welcome!

petablint complains about using observables in the noiseFormula. saCeSS (and I believe parPE) did not have a problem with that. This can be changed but will be less concise.

Right, it's supported by amici/parpe, but unfortunately not in PEtab v1 (PEtab-dev/PEtab#543). Will be supported in the next PEtab version. I guess for now it would be better to substitute the observable IDs.

@paulflang
Copy link
Contributor Author

Thanks @dweindl , I pushed the substitution.

@m-philipps m-philipps removed their assignment May 9, 2024
@m-philipps m-philipps self-assigned this May 9, 2024
@m-philipps
Copy link
Collaborator

m-philipps commented May 9, 2024

Hi Paul, thank you for the contribution, it is very welcome!

I'm checking the model based on this checklist and already added the simulated data and updated the visualisation specification to match Figure 7 in your publication. Is that fine with you?

It would be great if you could provide some information on the points below in the corresponding issue #210

  • A brief model description (one or two sentences)
  • A brief data description (one or two sentences)
  • 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)

and if you could check this point:

  • The model ID and model name attributes in the SBML model file match the problem name (example)

Thank you already!

Copy link
Collaborator

@m-philipps m-philipps left a comment

Choose a reason for hiding this comment

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

Thank you, this is ready to go!

@m-philipps m-philipps merged commit e50299b into Benchmarking-Initiative:master May 12, 2024
3 checks passed
kDipACdc log 0.008660178 86.60356004 0.812910086 1 uniform
kAspACdc log 1.17E-10 1.82E-07 1.06E-09 1 uniform
kPhApcA log 3.19E-14 3.19E-12 2.60E-13 1 uniform
kDpApc_1 log 0 0.03 0 1 uniform
Copy link
Member

Choose a reason for hiding this comment

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

Hi @paulflang, the following parameters are set log-scale, but have 0 as nominalValue, is this correct?

kDpApc_1
kDpE2f1
kPhC25A
kPhNup
kDpNup_1
kIm_2
kEx_2

Three out of those are marked as estimated. Shouldn't they be different from 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm. So the last four are set to zero and not estimated (I wanted to switch nuclear pore phosphorylation and unspecific nuclear import and export off).

For the first three, I always used zero as my guess (Note to myself: kDpApc_1 and kDpE2f1 are B55 independent background dephosphorylation rates. kPhC25A is CDC25 phosphorylation by CCNA. It is known that CCNB:CDK1 can phosphorylate CDC25, so I thought I let the optimizer figure out if CCNA:CDK1 can do it too). I just checked how David ran the optimization, and he just set the lower bound to -1e24 (in log space) and the optimal result for all three parameters was around -9.98e23. Bit strange that the optimizer did not move much from the lower bound in any of the cases.

Anyway, since the PEtab problem passes petablint, I think it is up to the optimizer how to handle zero lowerBound for log parameter scale. But I could also set lowerBound and nominalValue to a very low value, maybe 1e-100). @dweindl , what do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense. Thanks for the explanation, @paulflang.

since the PEtab problem passes petablint

There is the open question whether it should :).

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