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

Int64 take2 #5

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Int64 take2 #5

wants to merge 3 commits into from

Conversation

LJeub
Copy link
Contributor

@LJeub LJeub commented Oct 1, 2021

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
Copy link
Owner

c-f-h commented Oct 6, 2021

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
Copy link
Contributor Author

LJeub commented Oct 8, 2021

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
Copy link
Owner

c-f-h commented Oct 12, 2021

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