Remove python

[richterjakob]

Closes #28.

On hold until #33 is merged.

[richterjakob]

This PR is almost ready. I am waiting for #33 to be merged to rebase this branch to the new master one and resolve all merge conflicts.

[richterjakob]

Not ready to do this yet. See discussion on #28

Is this really in conflict with the discussion in #28? Everything in mentioned there is referring to the C++ code, so removing the Python parts shouldn't be a problem right?

[menon-karthik]

Not ready to do this yet. See discussion on #28

Is this really in conflict with the discussion in #28? Everything in mentioned there is referring to the C++ code, so removing the Python parts shouldn't be a problem right?

Yeah that's a good point. I guess we can go ahead. But let's not close #28 when merging this pull request yet. Because although #28 started off as "remove python solver", it now also includes discussions on moving svZeroDPlus to the Simvascular repository and replacing the Python solver there (which is even older than the one here I think?). That was my fault because I started that discussion in #28 instead of creating a separate issue for replacing svZerDSolver in Simvascular with svZeroDPlus.

[menon-karthik]

Not ready to do this yet. See discussion on #28

Is this really in conflict with the discussion in #28? Everything in mentioned there is referring to the C++ code, so removing the Python parts shouldn't be a problem right?

Yeah that's a good point. I guess we can go ahead. But let's not close #28 when merging this pull request yet. Because although #28 started off as "remove python solver", it now also includes discussions on moving svZeroDPlus to the Simvascular repository and replacing the Python solver there (which is even older than the one here I think?). That was my fault because I started that discussion in #28 instead of creating a separate issue for replacing svZerDSolver in Simvascular with svZeroDPlus.

Update: I created a separate issue for replacing Simvascular/svZeroDSolver with svZeroDPlus. So we can go ahead and close #28 when this is ready. Let me know when it's ready to review!

[mrp089]

Rebasing completed! @menon-karthik ready for your review.

[richterjakob]

Quick note: Before you merge, I need still need to update the documentation for the Python API, so that the complete interfaces is quickly mentioned and fix a small bug that I haven't found a good solution to yet.

[menon-karthik]

Sounds good! Let me know when it's ready.

[richterjakob]

This should be ready now

[menon-karthik]

This should be ready now

I will review it in the next few days! Thanks @richterjakob !

[menon-karthik]

@richterjakob I was wondering if we should add a note in the pip install documentation about installing in a conda environment? If that's too much work, don't worry about it!

[mrp089]

From what @richterjakob told me, the destructor inside the Python API was throwing random segfaults. This is a known issue when using std::unique_ptr with pybind11: https://github.com/pybind/pybind11/issues/1138. It seems like @richterjakob fixed that by returning the Solver object directly instead of a std::unique_ptr within PYBIND11_MODULE.

[mrp089]

He also changed the Python API to directly return a pandas.DataFrame object since we were converting to that anyways after running a simulation.

[mrp089]

@menon-karthik I think this is ready to merge.

Regarding the documentation, I think we don't require conda to install svzerodplus (I personally stopped using conda and switched to pyenv). Since we have pip install instructions, I think it's fine to not talk about conda.

[menon-karthik]

@menon-karthik I think this is ready to merge.

Regarding the documentation, I think we don't require conda to install svzerodplus (I personally stopped using conda and switched to pyenv). Since we have pip install instructions, I think it's fine to not talk about conda.

Oh I meant mention something about using any environment management tool while installing (not required, but recommended). Just thought it might be a good idea because most open-source packages I've seen with python/pip-based installation say something about using environments in their installation instructions, even if it isn't required. I'm sort of imagining in the very very very.... distant future when other people are using this, we might want to preempt issues being created about conflicting dependencies with other software. But yes, it's not super important.

[menon-karthik]

Also, sorry I thought I had already approved this pull request. @richterjakob Feel free to merge!

PS: make sure @mrp089 doesn't steal all the credit this time :)