Add DiffSL export functionality#5370
Conversation
* Fix typo in concentration description in notebook * Add CHANGELOG.md entry for typo fix * Remove unneccesary changelog entry Co-authored-by: Agriya Khetarpal <74401230+agriyakhetarpal@users.noreply.github.com> --------- Co-authored-by: Agriya Khetarpal <74401230+agriyakhetarpal@users.noreply.github.com>
main -> develop
* fix `InputParameter` serialisation * Update CHANGELOG.md
…-fix Don't be too strict with func_args longer than symbol.children
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
* feat: add`silence_sundial_warnings` solver option * refactor: `silence_sundials_warnings` -> `silence_sundials_errors`
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
* raise `SolverError` at failure to init sundials * Update simulation.py * Update idaklu_solver.py * reuse `pybammsolvers` error messages * Update test_idaklu_solver.py * bump `pybammsolvers` * Update CHANGELOG.md * Update CHANGELOG.md Update CHANGELOG.md
Co-authored-by: martinjrobins <martinjrobins@gmail.com>
BradyPlanden
left a comment
There was a problem hiding this comment.
This is looking good @martinjrobins, a few small areas to discuss. How are you envisioning the pydiffsol dependency is maintained? At the moment it's unpinned, which could lead to failing pybamm CI. Perhaps an upper bound on a major release?
don't you want a failing CI if your software ceases to be compatible with the latest version of a dependency, to let you know? I dunno, I can see arguments either way, you also want a dependable ci and only deal with breaking dependencies when you manually update them. I've also heard a security perspective that you should never pin if you can help it, so you get the latest security updates from direct and transitative dependencies. I'm close to the fence, but my feeling would be to leave it unpined. Saying that, I'm happy to pin to a major release if you think that is better. |
|
Not having an upper-bound pin is the way to go. Good read: Should You Use Upper Bound Version Constraints? |
|
Since My vote would be for a compatibility operator that can be bumped with Something like: |
Is there a way for users (using a dev install of pybamm) to install a newer version of pydiffsol than the upperbound we mandate in the same virtual environment? Same with bpx I guess since it has an upper pin. |
|
presumably we can just make sure our CI only ever installs pydiffsol 0.3.0, that way you don't have to worry about failing tests due to pydiffsol API changes |
|
Thinking on this further, how about moving the integration tests into |
hmmm, it would be slightly awkard development though. You would have a bunch of code in pybamm that is only tested in pydiffsol. We could keep the integration tests and remove the pydiffsol validation, keep them as simple snapshot tests to verify the output doesn't change, but this will give a lot of false positives when we change any of the pybamm models. What is the downside of only testing against pydiffsol==0.3.0 in the ci but not having the upper bound pin in pyproject.toml? That way you ensure that your CI never breaks due to updates in pydiffsol, and users can install whatever version of pybamm and pydiffsol they want. We could do the same with bpx as well |
|
Perhaps I'll remove the integration tests for now and put some snapshot tests in the unit tests to ensure that the exporter doesn't change functionality over time. We can keep discussing this, but it would be good to get the exporter in for now. |
…nto i5262-diffsl-export
Description
This PR adds the ability to export PyBaMM models to the DiffSL format, enabling models to be used with the DiffSL solver ecosystem.
Note this was started before the move from
developtomain, in the process some commits got added, but these don't affect the diffChanges
New
DiffSLExportclass insrc/pybamm/expression_tree/operations/diffsl.py:Unit and integration tests:
test_ode)test_heat_equation)Related Issues
Closes #5262
Checklist