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

Add PyBaMM IDAKLU Solver #176

Conversation

BradyPlanden
Copy link
Member

@BradyPlanden BradyPlanden commented Jan 26, 2024

Closes #160. WIP investigating implementation and performance.

@BradyPlanden BradyPlanden linked an issue Jan 26, 2024 that may be closed by this pull request
@agriyakhetarpal
Copy link
Member

Doesn't support native Windows, but should support WSL

The IDAKLU solver does support native Windows and the compilation outputs a .pyd file for it in the wheel. The PyBaMM documentation is incorrect in this regard, since it mentions building from source is unavailable, but the correct statement would be to say that the procedure is a bit more complex in comparison to that for Linux and macOS

@agriyakhetarpal
Copy link
Member

agriyakhetarpal commented Feb 11, 2024

PSA: PyBaMM's IDAKLU solver is broken on Windows currently for 24.1 and might work with earlier versions, see pybamm-team/PyBaMM#3783 and pybamm-team/PyBaMM#3100. The Linux and macOS wheels do not have this issue (edit: they do too)

@agriyakhetarpal
Copy link
Member

For now, a suitable way to help unblock this PR could be to build PyBaMM from source by checking out from a particular tag on the PyBaMM repository (if testing a particular PyBaMM version is needed, say, v24.1). On Windows, it can take up to 15 mins to compile, but Linux and macOS (both x86_64 and M-series for the latter) are pretty fast and it should add about ~2 mins to the jobs for the entire compilation. Now that #204 is complete, it won't take to run the tests either and the CI time should stay below 10 minutes, though the Windows jobs would take around ~20mins (but will still fail though as far as I am aware due to a single .pyd file, but the Linux and macOS jobs will work). I am happy to make these changes.

@agriyakhetarpal
Copy link
Member

Of course, we are working to cut a 24.1post1 patch release come out to fix this, not sure when that can be

@BradyPlanden
Copy link
Member Author

Hi @agriyakhetarpal, agreed, let's proceed with building from source to unblock this PR. If you can integrate that into the CI, that would be great. Once v24.1post1 is released, we can switch back to the wheels as you mentioned.

though the Windows jobs would take around ~20mins (but will still fail though as far as I am aware due to a single .pyd file, but the Linux and macOS jobs will work)

Shall we aim for linux and macOS for the time being?

Thanks again for looking into this!

@agriyakhetarpal
Copy link
Member

agriyakhetarpal commented Feb 26, 2024

That should work, we can skip the Windows build. I can write a PR to this branch where I can add PyBaMM to be compiled from source from the v24.1 tag, merging that should bring the changes here. We could test against PyBaMM from the develop branch too as needed (which would be more relevant for the nightly builds, however).

@agriyakhetarpal
Copy link
Member

How do you feel about not using nox in the workflow files and re-using the workflow commands? I ask because it would be cumbersome to edit noxfile.py to compile PyBaMM inside it, my intention is to re-use the pytest-related commands from noxfile.py since there aren't a lot. The nox session would potentially overwrite PyBaMM with its PyPI installation, and there is no way to control the nox sessions before they have been created, i.e., from outside the locus of noxfile.py. This would limit the usage of nox a bit, but the Scientific Python guides on task runners advocate for not using it in CI (not that anyone pays heed to that guideline anyway)

@BradyPlanden BradyPlanden added the blocked This issue or pull request is blocked label Apr 27, 2024
@BradyPlanden BradyPlanden deleted the 160-update-basemodelsimulate-for-idaklu-output_variables branch May 28, 2024 11:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked This issue or pull request is blocked
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update BaseModel.simulate() for IDAKLU output_variables
2 participants