Add a test for what petab does with a derived BNGL parameter - #794
Merged
Merged
Conversation
PyBNF declares petab>=0.9,<1 and CI resolves that range on every run, so a change in petab arrives here without anyone editing this repository. The only PyBNF tests that touch petab's BnglModel parameter accessors run against a model whose three parameters are plain numbers, so nothing pinned the case that matters: a parameter whose value is an expression over other parameters, written as kon koff/Kd. That case matters because a PEtab parameter table may override or estimate the parameters such a value is computed from. A value reported from the model file alone would reach a simulator as a constant, override the model's own expression, and stay at the stale number. PyBNF's own exporter already refuses to give such a parameter a nominal value, in _numeric_nominal, and this test says that petab must not contradict it. The test passes against released petab 0.9.0, which refuses the value with NotImplementedError, and against the change proposed in PEtab-dev/libpetab-python#517, which refuses it with ValueError. Both leave the parameter out of the list of free parameter values, which is the part PyBNF relies on.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A BioNetGen parameters block can give a parameter an expression over other
parameters instead of a number, for example
kon koff/Kd. Call that a derivedparameter. PyBNF reads BioNetGen models through the petab library when it works
with PEtab problems, and petab's BnglModel has two methods that report parameter
values.
The problem this test addresses is that nothing here pinned what those two
methods do with a derived parameter. The existing tests in TestNativeBnglModel
run against the exported demo model, whose three parameters are plain numbers,
so they say nothing about the case. That matters because a PEtab parameter table
may override or estimate the parameters a derived value is computed from. If
petab reported a value taken from the model file alone, that value would reach a
simulator as a constant, override the model's own expression, and stay at the
stale number while the parameters it depends on are fitted. PyBNF's own exporter
already refuses to give a derived parameter a nominal value, in
_numeric_nominal, so the two sides agree today, silently and by accident.There is no version gate that would let us notice a change. PyBNF declares
petab>=0.9,<1in pyproject.toml and in the setup action,uv.lockis notcommitted, and continuous integration resolves that range on every run. Any
petab release below 1.0 is picked up automatically. The new test is the only
thing that would notice petab starting to report a value for a derived
parameter.
What the test asserts is the part that has to hold either way: a derived
parameter does not appear in the list of free parameter values, a plain number
still does, and asking for the derived parameter's value raises. It accepts
either ValueError or NotImplementedError, because released petab 0.9.0 raises
the second and the change proposed upstream in PEtab-dev/libpetab-python#517
raises the first.
Verification. The assertions were run against both versions of petab, selecting
each source tree with PYTHONPATH and printing the directory petab was actually
imported from, since a version string alone does not prove which copy is loaded.
Against released 0.9.0 the free values are
{'koff': 0.1, 'Kd': 5.0}and thevalue request raises NotImplementedError. Against the upstream branch the free
values are the same and the request raises ValueError.
One caveat worth recording: this test cannot be run in the local development
environment as it stands, because that environment holds petab 0.8.2 while
pyproject.toml asks for 0.9 or later. Six tests in TestNativeBnglModel already
fail there for the same reason, since petab 0.8.2 has no BioNetGen loader at
all. The cause is a stale uv.lock, which is not tracked in the repository, and
uv syncfixes it. Continuous integration installs the declared range and isunaffected.