Download IDAKLU from pybammsolvers#4487
Conversation
|
A new link error cropped up, but it looks like we could get a lot of savings on time with this update. Edit: Most of the run time appears to be in the integration tests, so unfortunately the time savings are not as good as I would have hoped. |
|
The linkage error is the same one as #3783, coming from CasADi's plugin system. I am not sure if it's worth fixing it, since it was fixed by @martinjrobins for the linear interpolant case by dropping down to Python but IIRC there wasn't a way in CasADi for doing it for the cubic |
|
@agriyakhetarpal Yeah I was looking at that issue as well. As far as I can tell CasADI sets a path for plugins. I am trying to see if there is a decent workaround since this was part of #4464 My guess is that the wheels for the next release will be broken as well, but I have not confirmed it yet |
|
There is a workaround for Linux and macOS, but not for Windows (different toolchain); sadly, it's not decent enough to include. I think I'll raise a PR upstream in CasADi to get one part of the linkage going and see if we can migrate to a non-MSVC toolchain (which can potentially help provide that workaround for this on Windows later on). It's been on my list of things to do for a while, but I've yet to do it. |
|
This is fixed locally with this: |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #4487 +/- ##
===========================================
- Coverage 99.22% 98.66% -0.56%
===========================================
Files 303 303
Lines 23070 23224 +154
===========================================
+ Hits 22891 22914 +23
- Misses 179 310 +131 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Yes, won't work with Windows |
Saransh-cpp
left a comment
There was a problem hiding this comment.
This is fantastic. Took a quick view and it looks good overall!
Co-authored-by: Agriya Khetarpal <74401230+agriyakhetarpal@users.noreply.github.com>
Co-authored-by: Agriya Khetarpal <74401230+agriyakhetarpal@users.noreply.github.com>
Co-authored-by: Agriya Khetarpal <74401230+agriyakhetarpal@users.noreply.github.com>
|
@agriyakhetarpal I know we have not solved the ARM64 or conda-forge issues yet, but how do you feel about getting this merged ASAP to see if we start getting issues reported? |
agriyakhetarpal
left a comment
There was a problem hiding this comment.
Yes, let's do that, @kratman. Linux arm64 is quite common but lacks readily available CI providers for OSS; Windows arm64 CI is not present either outside of Windows Arm dev boxes. FWIW, we can mark both platforms/architectures as unsupported for now and ask users to compile it themselves (the same as we did when macOS arm64 was not common).
Based on https://stackoverflow.com/a/64921347, we will have to wait a bit for PyBaMM 25.1 to be supported on conda-forge until conda-forge/staged-recipes#28748 is resolved, but at the time if someone needs a new version of PyBaMM in a conda environment, they should be able to pip-install without any issues as long as their environment files specify that.
|
Note: The changes from #4736 are not in pybammsolvers yet. The tests pass without the C++ side for now though. I am working on the pybammsolvers v0.0.5 release, but I have to do a bit of testing before it is ready. Hopefully I will finish that off today |
Description
This will separate the IDAKLU C++ code from pybamm.
Fixes #3564
Fixes #4611
This ticket is something that can be handled in the other repo
Closes #3603
Type of change
This should speed up CI by skipping the build of the C++ code.
Key checklist:
$ pre-commit run(or$ nox -s pre-commit) (see CONTRIBUTING.md for how to set this up to run automatically when committing locally, in just two lines of code)$ python run-tests.py --all(or$ nox -s tests)$ python run-tests.py --doctest(or$ nox -s doctests)You can run integration tests, unit tests, and doctests together at once, using
$ python run-tests.py --quick(or$ nox -s quick).Further checks: