Petab Importer reforge #1442

PaulJonasJost · 2024-08-03T09:49:29Z

CHanged the way the petab importer works. It used to be tailored to amici models and contain a lot of functionality that were only necessary for objective function creation of amici. As we now support other simulators, changed it to a Importer-Factory design.

The importer only creates the problem now. You can however supply a simulator_type to it, which then creates the corresponding factory, creating the objective. All Functions that are simulator specific are in the factory.

This should make the Code more clear in my opinion, we are getting rid of the PetabImporter depending on the AmiciObjectBuilder and make it much more versatile to support a new simulator.

Also added a basic support for any PEtab simulator. This allows every simulator who is supporting PEtab to directly use pyPESTO. This is not an efficient implementation, but it is also not meant to be one, rather lowering hurdles to use pyPESTO.

Check that all tests work fine and adapt to new code structure

…eaving the importer really independent of amici

… AmiciFactory

…n with different models. Created a basic PetabSimulator supported (inefficent most likely but as a starter)

codecov-commenter · 2024-08-03T10:10:12Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 88.59649% with 52 lines in your changes missing coverage. Please review.

Project coverage is 82.87%. Comparing base (2e57687) to head (809a8e0).

Files with missing lines	Patch %	Lines
pypesto/petab/objective_creator.py	88.88%	29 Missing ⚠️
pypesto/petab/importer.py	69.23%	12 Missing ⚠️
pypesto/objective/petab.py	90.47%	4 Missing ⚠️
pypesto/petab/util.py	94.44%	3 Missing ⚠️
.../objective/roadrunner/petab_importer_roadrunner.py	87.50%	2 Missing ⚠️
pypesto/objective/roadrunner/road_runner.py	87.50%	2 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #1442      +/-   ##
===========================================
- Coverage    83.44%   82.87%   -0.57%     
===========================================
  Files          160      163       +3     
  Lines        13502    13760     +258     
===========================================
+ Hits         11267    11404     +137     
- Misses        2235     2356     +121

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

… solution to amici Objective

…tab_simulator_support

dweindl

Thanks. Some first feedback:

Overall, it makes sense to split up the PEtab-related functionality.

I am not sure how much demand there really is for the petab.Simulator support, but fine to have it. (It should be relatively easy to include a new simulator, but for really using it during parameter estimation, one would probably want a more efficient implementation).

I am not so happy with the naming yet, see specific comments.

doc/example/getting_started.ipynb

pypesto/petab/factory.py

dweindl · 2024-08-09T10:33:54Z

pypesto/petab/factory.py

+                        )
+        return mapping
+
+    def create_objective(


I think anything besides this function could/should be private.

Could if we view the sole purpose of the factory as the objective generation. If we view the other objects like datas and/or model part of its tasks as well, they should not be private. The petabImporter had them previously also as not private. If we want them to be private, there would be no need to have a lot of arguments in them allowing to separately call them as they anways were not meant to be used that way?

I will have to have a closer look what might be useful elsewhere. Not everything that was public before should have been public before. If the other functions are to be used separately, then they should not be in this class, or this class should not be called ObjectiveFactory or similar. A XFactory has (or should have) a single purpose, which is to produce an instance of X.

Agreed, in this case we can make them private any day (would also introduce potential breaking changes?) i am also very much in favor of removing some of these functions at least in the future as i dont really see a point in such light wrappers (see e.g. rdatas_to_measurement_df). For now i moved everything away from the importer, but happy to also remove not needed functions (i already removed check gradients match).

pypesto/petab/importer.py

dweindl · 2024-08-09T10:48:47Z

pypesto/petab/importer.py

-            rdatas, model, measurement_df
+        """See :meth:`AmiciFactory.rdatas_to_measurement_df`."""
+        raise NotImplementedError(
+            "This function has been moved to `AmiciFactory`."


I don't think this function really belongs to AmiciFactory. Also this would be a breaking change without deprecation warning, which would require pypesto 1.0 (there are a couple more cases like that).

i'll be honest, i dont think they even belong to pyPESTO, as it is a really light wrapper around it. Can create a deprecation warning as well.

Why would it require a 1.0? we could go with a 0.6.0 as we have done before on breaking changes?

… the parameter update function

…tab_simulator_support

dilpath

Overall, looks nice, thanks 🙂

doc/example/petab_import.ipynb

pypesto/petab/factory.py

pypesto/petab/importer.py

dilpath · 2024-08-29T15:27:26Z

pypesto/petab/importer.py

+        self.simulator_type = simulator_type
+        self.simulator = simulator


As the importer only needs to use this in create_objective, why not remove these attributes and implement them as arguments of create_objective instead?

e.g. if I want to compare the libroadrunner and AMICI objectives, why do I need two importer objects? I could use the same importer like importer.create_objective_creator(..., simulator_id=AMICI) and importer.create_objective_creator(..., simulator_id=ROADRUNNER)

No strong opinion on this though, fine as is

+1
Think this makes sense. Also not strong opinion.

having this would make it imo much easier to

init the importer -> 2. importer.create_problem()
without having to pass these arguments constantly to create_problem. Could add it as additional possibility to create_objective such that people can do either? Argument in create_objective would then take precedence

IMO the cleanest implementation would be that default behavior (AMICI simulator) is usable as you said like

importer = PetabImporter(petab_problem) problem = importer.create_problem()

but custom behavior is like

importer = PetabImporter(petab_problem) obj = importer.create_objective(AMICI) obj.amici_solver.setAbsoluteTolerance(1e-8) problem = importer.create_problem(obj)

Since it's quite common for me to customize things like simulator tolerances, passing obj to create_problem is no change for me.

Just to explain my (weak) preference.

passing the objective to problem is still possible. And the other behavior would look like (+- details)

importer = PetabImporter(petab_problem, AMICI) obj = importer.create_objective() obj.amici_solver.setAbsoluteTolerance(1e-8) problem = importer.create_problem(obj)

I think going with allowing both would be best solution to be able to adjust to individual preferences?

I also prefer to only provide users with one way of doing things 😅 So my preference would be to only permit specification via __init__ xor create_objective, and between those I prefer create_objective.

Since you have done the work, I'll defer to whatever you decide.

By now, PetabImporter doesn't have much functionality or internal state left. Maybe we could directly go for something like

importer = AmiciPetabImporter(petab_problem, ...) problem = importer.create_problem() ...

By now, PetabImporter doesn't have much functionality or internal state left. Maybe we could directly go for something like

importer = AmiciPetabImporter(petab_problem, ...) problem = importer.create_problem() ...

I think currently the main advantage is that we can easily switch by just changing an argument in the Importer to switch simulators if needed/wanted which we would lose with the above. additionally the create_problem function is for all of them kind of the same which just in my mind creates this natural hierarchy of "Importer -> Inner thing (=Objective creator)"

easily switch by just changing an argument

In the other case, it's just changing the class name.
It has the advantage of better static code analysis support and avoids having PetabImporter.__init__ with the superset of arguments for all simulators.

You mean like this?

class AmiciPetabImporter(PetabImporter): def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) def create_objective(self, *args, **kwargs): super().create_objective(simulator_type=AMICI, *args, **kwargs)

Fine for me. Having the argument in the PetabImporter.create_objective makes sense to me since that's where it's used, but the other helpful alternatives (like PetabImporter(simulator_type=...) or AmiciPetabImporter) also make sense 👍

doc/example/petab_import.ipynb

Co-authored-by: Dilan Pathirana <[email protected]>

Doresic

Looks great. Good changes.

Doresic · 2024-09-11T12:13:00Z

pypesto/petab/importer.py

+        self.simulator_type = simulator_type
+        self.simulator = simulator


+1
Think this makes sense. Also not strong opinion.

pypesto/petab/importer.py

doc/example/petab_import.ipynb

Co-authored-by: Fabian Fröhlich <[email protected]>

…_creator

PaulJonasJost · 2024-09-16T09:48:45Z

As no one had a strong opinion, I would put it up to vote how to organize the code:

Keep the PetabImporter and the objective creator separate but don't enforce setting the simulator_type in the init of the importer, rather also allowing it to be put in the create_objective_creator as well. (Vote with 🎉)
Mentioned Up-/Downsides:
- (+) Clear separation of tasks, one creates the objective and anything auxiliary, the other only has to create the problem from the petab problem
- (-) More overloading of arguments in the PetabImporter
- (+) Easy switching of underlying objective and/or potential to easily create two different problems to compare
For each simulator type create its own SimulatorPetabImporter (Vote with 🚀 )
Mentioned Up-/Downsides:
- (+) Loose some arguments and no overload of arguments
- (+/-) Clear functionality tailored to exactly one simulator type
- (-) Strong break with current codepipelines. Any current code having previously used the importer will have to be revised

I would keep the poll open till end of tomorrow :)

PS: sorry for the emojis of vote, these were the two most neutral ones I saw one can react with under a comment.

PaulJonasJost added 5 commits July 31, 2024 12:17

Moved creation of the amici objective function to a separate class, l…

cab5b27

…eaving the importer really independent of amici

Remove check gradients, as this can be done in an objective function

cf348c1

Added functions with a not implementedError and moved most of them to…

3775c3c

… AmiciFactory

Moved import to a factory, allowing a single PEtabImporter to functio…

68f365b

…n with different models. Created a basic PetabSimulator supported (inefficent most likely but as a starter)

Resolve potential annotation problems.

d214852

PaulJonasJost and others added 15 commits August 5, 2024 11:13

Solved the problem of x_names not saving the fixd parameters. similar…

7482672

… solution to amici Objective

rewrote petab testsuite to fit new design.

cf2edcd

Adjusted text and example code for new changes

385030b

import error with petab simulator

10c6765

updated code

722d637

import petab as optional

f0f672f

Adjusted roadrunner example and also solved issue in PetabImporterRR

a321a39

Another import

865c3cc

Another import and spline error

cd96ac0

Merge branch 'develop' into petab_simulator_support

94ed171

Changed the documentation that is included

c9f9f7b

Merge remote-tracking branch 'origin/petab_simulator_support' into pe…

32f930d

…tab_simulator_support

added test for petabSimulator with the example of basico

33e1a33

Fix text for check gradients and fix roadrunner notebook.

1ccdbf2

added test for petabSimulator with the example of basico

b80b4c8

dweindl reviewed Aug 9, 2024

View reviewed changes

PaulJonasJost added 2 commits August 9, 2024 14:50

cleared output

ce91536

Hopeflly renamed everything

9bbdb13

PaulJonasJost marked this pull request as ready for review August 9, 2024 13:35

PaulJonasJost requested review from FFroehlich, Doresic, m-philipps and dilpath as code owners August 9, 2024 13:35

Temporary fix for roadrunner fd objective

fef1387

PaulJonasJost self-assigned this Aug 14, 2024

PaulJonasJost and others added 6 commits August 23, 2024 10:06

correct installation of basico as petab simulator in test

662de64

Removed try-except in simulator as it is to unspecific. Now requiring…

a34a54b

… the parameter update function

Merge branch 'develop' into petab_simulator_support

40269ec

not basico but copasi-basico

e21533d

Merge remote-tracking branch 'origin/petab_simulator_support' into pe…

5293eef

…tab_simulator_support

not copasi-basico but copasi-basico[petab]

77bb2bb

dilpath approved these changes Aug 29, 2024

View reviewed changes

PaulJonasJost and others added 5 commits September 4, 2024 13:53

Merge branch 'develop' into petab_simulator_support

8962a52

Adjusted some description text

f5f5d39

Merge branch 'develop' into petab_simulator_support

479ba3e

More accurate documentation

53c8e61

Replaced Usage of roadrunner, amici, petab strings with constants

4c4e978

dilpath reviewed Sep 9, 2024

View reviewed changes

doc/example/petab_import.ipynb Outdated Show resolved Hide resolved

Update doc/example/petab_import.ipynb

5e5be6d

Co-authored-by: Dilan Pathirana <[email protected]>

PaulJonasJost mentioned this pull request Sep 9, 2024

Roadrunner does not correctly handle estimated noise parameters #1441

Open

dweindl linked an issue Sep 9, 2024 that may be closed by this pull request

Decouple PEtab and AMICI #1301

Open

Merge branch 'develop' into petab_simulator_support

2831e47

Doresic approved these changes Sep 11, 2024

View reviewed changes

FFroehlich approved these changes Sep 13, 2024

View reviewed changes

doc/example/petab_import.ipynb Outdated Show resolved Hide resolved

PaulJonasJost and others added 3 commits September 16, 2024 10:50

Update doc/example/petab_import.ipynb

20057d5

Co-authored-by: Fabian Fröhlich <[email protected]>

Renamed factory mentions as it was not a fitting name.

460213b

added option to pass simulator type and simulator to create_objective…

fde7b07

…_creator

PaulJonasJost added 3 commits September 16, 2024 11:50

Merge branch 'develop' into petab_simulator_support

a09e8bf

Merge branch 'develop' into petab_simulator_support

983da42

Merge branch 'develop' into petab_simulator_support

809a8e0

PaulJonasJost merged commit 0264a1e into develop Sep 24, 2024
18 checks passed

PaulJonasJost deleted the petab_simulator_support branch September 24, 2024 09:14

PaulJonasJost mentioned this pull request Oct 30, 2024

Release v0.5.4 #1468

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Petab Importer reforge #1442

Petab Importer reforge #1442

PaulJonasJost commented Aug 3, 2024 •

edited

Loading

codecov-commenter commented Aug 3, 2024 •

edited

Loading

dweindl left a comment

dweindl Aug 9, 2024

PaulJonasJost Aug 9, 2024

dweindl Aug 9, 2024

PaulJonasJost Aug 9, 2024

dweindl Aug 9, 2024

PaulJonasJost Aug 9, 2024

dilpath left a comment

dilpath Aug 29, 2024

Doresic Sep 11, 2024

PaulJonasJost Sep 11, 2024

dilpath Sep 11, 2024

PaulJonasJost Sep 11, 2024

dilpath Sep 11, 2024

dweindl Sep 11, 2024

PaulJonasJost Sep 11, 2024

dweindl Sep 11, 2024

dilpath Sep 11, 2024

Doresic left a comment

Doresic Sep 11, 2024

PaulJonasJost commented Sep 16, 2024

		self.simulator_type = simulator_type
		self.simulator = simulator

Petab Importer reforge #1442

Petab Importer reforge #1442

Conversation

PaulJonasJost commented Aug 3, 2024 • edited Loading

codecov-commenter commented Aug 3, 2024 • edited Loading

Codecov Report

dweindl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dilpath left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Doresic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PaulJonasJost commented Sep 16, 2024

PaulJonasJost commented Aug 3, 2024 •

edited

Loading

codecov-commenter commented Aug 3, 2024 •

edited

Loading