ilupp icon indicating copy to clipboard operation
ilupp copied to clipboard

Int64 take2

Open LJeub opened this issue 3 years ago • 3 comments

Here is my attempt at implementing the two-module solution. It is almost entirely transparent to the user and does not require any upcasting. The only difficulty is supporting the iluplusplus_precond_parameter and preprocessing_sequence objects as these are not constructed with a sparse matrix as input and thus have no way of automatically determining the appropriate module to use. My current work-around requires the user to specify the type manually for use with 64bit matrices.

LJeub avatar Oct 01 '21 13:10 LJeub

This looks good so far. For the precond_parameter class, most of the Integer fields there are actually just de-facto enums and could simply be int. For the remaining ones, it should be fine to use int64 in both instances of the library, then the same class can be used in both versions.

c-f-h avatar Oct 06 '21 09:10 c-f-h

I agree that it would be nice to have only a single version of the parameter class. After re-reading the pybind11 documentation again (this is the relevant page: https://pybind11.readthedocs.io/en/stable/advanced/classes.html) it may actually be quite easy to get this to work. Note that I had to add py::module_local to all the bindings to be able to compile them into two different modules. While this means we have two different parameter classes on the Python side, both versions are actually accepted by the bindings when passing from Python to C++. Right now, using mismatched parameter classes just results in an error due to the mismatched types. However, if we make sure that the parameter classes are exactly equivalent, whether the library is compiled with 64bit indices or not, this should all just work.

So, changing all the Integer fields in the parameters to be hard-coded to either int or int64 and then importing one of the two versions in the python code should make everything work exactly as before and make the switch to 64bit indices completely transparent to the user.

LJeub avatar Oct 08 '21 13:10 LJeub

So, changing all the Integer fields in the parameters to be hard-coded to either int or int64 and then importing one of the two versions in the python code should make everything work exactly as before and make the switch to 64bit indices completely transparent to the user.

I agree! That's exactly what I had in mind. Is that something you are interested in doing? I find the current state of the PR a bit too messy from the user's perspective, with having to specify the index type manually, but if that could be made transparent again, I would definitely merge it.

c-f-h avatar Oct 12 '21 09:10 c-f-h