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

Remove deprecated functionality for PEtab import from individual files instead of petab.Problem #2458

Closed
dweindl opened this issue Jun 16, 2024 · 0 comments
Assignees
Labels
PEtab PEtab-import related

Comments

@dweindl
Copy link
Member

dweindl commented Jun 16, 2024

Remove deprecated functionality for PEtab import from individual files instead of petab.Problem:

def import_model_sbml(
sbml_model: Union[str, Path, "libsbml.Model"] = None,
condition_table: str | Path | pd.DataFrame | None = None,
observable_table: str | Path | pd.DataFrame | None = None,
measurement_table: str | Path | pd.DataFrame | None = None,
petab_problem: petab.Problem = None,

#2456 showed that we need the parameter table for proper PEtab import, but there is no option to pass the parameter table as individual file (since in the early days, we didn't (think we'd) need it). To avoid any surprises and wrong gradients there, I'd suggest to remove this functionality as part of fixing #2455.

There has been a DeprecationWarning in place for almost 2 years (#1810) and I think this isn't used anyways.

Independent of this specific issue, I think it would be good to allow removing deprecated functionality not only in major releases, but also in minor releases after a certain transition period (maybe 1y after the first release containing the deprecation warning? after 3 minor releases? ...? See also, for example, numpy's policy). Opinions? @FFroehlich?

@dweindl dweindl added the PEtab PEtab-import related label Jun 16, 2024
@dweindl dweindl mentioned this issue Jun 16, 2024
2 tasks
@dweindl dweindl self-assigned this Jun 16, 2024
dweindl added a commit to dweindl/AMICI that referenced this issue Jun 16, 2024
…s instead of petab.Problem

Since almost two years, it's possible to pass a `petab.Problem`, which is safer
and more convenient.
Omitting the parameter table for model import (since there is no option to supply
this) is likely to produce unwanted results (see AMICI-dev#2458 and AMICI-dev#2455 for more details),
and therefore, this functionality is best removed.
dweindl added a commit to dweindl/AMICI that referenced this issue Jun 25, 2024
As suggested in AMICI-dev#2458.

Other opinions?
dweindl added a commit that referenced this issue Jun 26, 2024
Add deprecation policy

As suggested in #2458.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PEtab PEtab-import related
Projects
None yet
Development

No branches or pull requests

1 participant