PEtab-dev / libpetab-python

Python package for working with PEtab files
https://libpetab-python.readthedocs.io
MIT License
14 stars 5 forks source link

Incorrect check in linter on transformed parameter bounds #176

Closed matthiaskoenig closed 1 year ago

matthiaskoenig commented 1 year ago

This is related to https://github.com/PEtab-dev/PEtab/issues/546.

As I understood things we have to transform all values in the parameter table if the parameterScale is not lin. I.e. lowerBound, upperBound, nominalValue. When doing this we get the following error on the petab linter.

Bounds for log10 scaled parameter LI__ICGIM_Vmax must be positive.
Not OK

This is an incorrect check, because the transformed log bounds can be <0.

I.e. the linter checked what the documentation stated before. I think it does make much more sense to just provide the absolute values of the bounds & initial value and let the tools handle the conversion. But with the transformation the linter has to be updated.

Example attached.

parameterId parameterName   parameterScale  lowerBound  upperBound  nominalValue    estimate    initializationPriorType initializationPriorParameters   objectivePriorType  objectivePriorParameters
LI__ICGIM_Vmax  Vmax ICG import [mmole/min/l]   log10   -5.0    2.0 -1.4322694001146912 1   parameterScaleUniform   1E-5;1E2    parameterScaleUniform   1E-5;1E2
LI__ICGIM_Km    Km ICG import [mM]  log10   -4.0    0.0 -1.6643580171689705 1   parameterScaleUniform   1E-4;1  parameterScaleUniform   1E-4;1
LI__ICGLI2CA_Vmax   Vmax ICG to CA transport [mmole/min/l]  log10   -4.0    1.0 -3.0251785764973227 1   parameterScaleUniform   1E-4;10 parameterScaleUniform   1E-4;10
LI__ICGLI2CA_Km Km ICG to CA transport [mM] log10   -4.0    4.0 -1.9069756923812202 1   parameterScaleUniform   1E-4;1E4    parameterScaleUniform   1E-4;1E4
LI__ICGLI2BI_Vmax   Vmax ICG to BI transport [1/min]    log10   -5.0    1.0 -3.9408282503033405 1   parameterScaleUniform   1E-5;10 parameterScaleUniform   1E-5;10

icg_sd.zip

dilpath commented 1 year ago

The current docs state, for lowerBound, upperBound and nominalValue:

Must be provided in linear space, independent of parameterScale.

The next version of the docs will state

Take the [transformation] of the parameter value, lowerBound, upperBound, and nominalValue during parameter estimation.

and

The provided value will be transformed according to parameterScale.

i.e., values still need to be provided on linear scale

I think it does make much more sense to just provide the absolute values of the bounds & initial value and let the tools handle the conversion. But with the transformation the linter has to be updated.

This is how it's done currently. The related issue is that an end user might only see the transformed value in their tools and the output (e.g. plots) that the tool produces, if the tool does not convert values back to linear scale. However, this is often unrelated to libpetab-python.

NB: the nominal value is not the initial value (unless perhaps you are using these parameters to set the initial condition of state variables in the PEtab conditions table?).

matthiaskoenig commented 1 year ago

Hi @dilpath, the current docs are here which state the opposite https://github.com/PEtab-dev/PEtab/pull/547/files

Best Matthias

dilpath commented 1 year ago

Hi Matthias,

Yes, I meant to point out that the docs have remained consistent. The first quote is from the old docs, and the second and third quotes are from the current docs that you linked to. The wording is different but the meaning is still that everything is in linear scale in the PEtab files. The transformation to parameter scales occurs in the tools that use PEtab, hence e.g. "Take the [transformation] [...] during parameter estimation" and "The provided value will be transformed...". Maybe this can be stated more clearing in the docs (though, I guess that was what the PR you linked to tried to achieve :see_no_evil:). Is there a specific part that can be easily misinterpreted, or should it be rewritten? We could then fix the docs.

Kind regards, Dilan

matthiaskoenig commented 1 year ago

So I assume pypesto is implementing this incorrectly at the moment (i.e. does an incorrect/no conversion). I have to check back with @shoepfl on that. But we both understood that we have to convert the bounds in PETab, because the bounds were incorrect in pypesto.

Will close this after clarification.

shoepfl commented 1 year ago

Hi all,

as far as I understood is the confusing part that you have to provide the parameter values in linear scale but they are transformed by tool X (in our case pypesto). So if I understood correct we have to keep in mind that the linear bound we think of is transformed in the end and to think this transformation back - in order to get the right absolut bounds we have to back-transform the bounds right?

So as a simple example - Lets say the lower bound of parameter k1 should be 0.01. This parameter is log-transformed by the tool so i have to write as lower bound -2 in the parameter table right? Because log10(-2)=0.01 and this is the bound i want.

dilpath commented 1 year ago

Ah, log10(0.01) = -2. log10(-2) is undefined for real numbers, which is the same error in the first post.

Is this the same issue as https://github.com/ICB-DCM/pyPESTO/issues/881 ?

If so, then the problem is different, i.e. that pyPESTO doesn't label the x-axis correctly: "Parameter value" should be "log10(Parameter value)", since the log10-transformed values are plotted.

Just to clarify:

  1. specify everything on linear scale in PEtab
  2. tool will take values from PEtab and convert them. e.g. if 0.01 and log10 is in the PEtab files, then the tool will use -2 internally, which is correct and the intended use. This might mean you see -2 in plots, which means the plot is also with log10-transformed values, not values on linear scale.

If you want to plot with values on linear scale, then unscale the tool's output (do not try to resolve this in PEtab files as suggested or you will get incorrect output from the tool). This is usually easy, with helper methods like petab.Problem.unscale_parameters [1]. If it's some effort to unscale or you're not sure how, feel free to contact the tool's developers and ask how to do it easily.

[1] https://github.com/PEtab-dev/libpetab-python/blob/ff1bdde91ca1ebd9af0b0eaee17d3a472d920857/petab/problem.py#L752

shoepfl commented 1 year ago

Yes exactly the pypesto issue is related.

Ah okay, now i understand. So the value is only transformed when the programm is working on the different scale but not transformed in the sence that the value is actually altered overall. This makes sence - so the linear bounds are the "real" bounds.

@matthiaskoenig then indeed we do not have to scale our parameters before and the plot was made with log10-transformed values

Thanks @dilpath

matthiaskoenig commented 1 year ago

Perfect. Thanks.