Skip to content

Int64 take2 - #5

Open
LJeub wants to merge 3 commits into
c-f-h:masterfrom
LJeub:int64_take2
Open

LJeub wants to merge 3 commits into
c-f-h:masterfrom
LJeub:int64_take2

Conversation

@LJeub

@LJeub LJeub commented Oct 1, 2021

Copy link
Copy Markdown
Contributor

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.

…ize"

6ed9f51

This conflicts with switching between modules automatically
@c-f-h

c-f-h commented Oct 6, 2021

Copy link
Copy Markdown
Owner

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.

@LJeub

LJeub commented Oct 8, 2021

Copy link
Copy Markdown
Contributor Author

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.

@c-f-h

c-f-h commented Oct 12, 2021

Copy link
Copy Markdown
Owner

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants