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

Changed behaviour on how eigency deals with types #73

Merged
merged 7 commits into from
Dec 4, 2024

Conversation

Maki4748
Copy link
Contributor

@fbordeu This should give us the behaviour we want.
Tests are currently fixed width, tho I'm gonna add some aliases in a bit, with np.long being a big problem, cause it doesn't exist between 1.24.0 and 2.0.0.

On the c++ side the types are handled way differently, every possible type is defined for the Arrays, Maps and Matrices, so we only really give it the object over and it figures the correct type out on it's own.
Same happens in reverse, thanks to typed memoryviews cython deals with finding the correct type and gives it over to numpy.

I wanted to originally write a proposal for this one, since it is a very big change in how eigency deals with Eigen->Numpy, but this right now is the best way I could figure to solve the problem of giving numpy the correct type always and ever with consistent behaviour.

Please look it over.

Also, just to note, the whole mapping the correct function for NDAC and NDAV, I'm working on it, currently this is the best approach, but I'm not happy with this. I got some pointers from some cython devs in cython/cython#6525, but this is gonna take me a bit more googling to figure out.

@Maki4748
Copy link
Contributor Author

long and ulong are still missing in the tests, thanks to Numpy removing it in 1.24.0 and reintroducing it in a different way in 2.0.0 I need to use compile time conditions, but at the same time cython/cython#4310 is removing the nice way to implement this, so I'm gonna need to figure something out.

@Maki4748
Copy link
Contributor Author

Maki4748 commented Nov 30, 2024

@fbordeu I'm not entirely happy with this, but it works and doesn't use anything deprecated.

@fbordeu
Copy link
Contributor

fbordeu commented Nov 30, 2024

Wow, lots of work!! I'm going to read the code carefully on Monday. Can you share with me in what project you use eigency.

@Maki4748
Copy link
Contributor Author

Maki4748 commented Dec 1, 2024

Sure thing, take your time.
I'm working on porting a tool called WarpFactory over from Matlab and it needs to do a lot of calcs with tensors, which takes a lot of time with Python/Numpy and Eigen could help me solve that massive Bottleneck.
Every other Lib I had in mind in cpp didn't support one thing or the other or I would have to write a whole wrapper from scratch.
So this is sorta a whole side quest, but it's been fun, especially since I have to write tests for WarpFactory right now and I'd rather do anything else.

Tho I also wanted to work on eigency, cause I saw it wasn't maintained for a while and I figured out how hard it is to find a nice wrapper for these kind of implementations myself.
Besides that I wanted to do some coding for Open Source projects for a while now and the big names like Numpy or Scipy are very intimidating.

Copy link
Contributor

@fbordeu fbordeu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank for all the work, just some comments.

I Just merged a little patch to give the user the possibility to use his own version of eigen. So you can start your prototype for the tensors.

eigency/core.pxd Show resolved Hide resolved
_NDAV(std::complex<float>, ndarray_complex_float, ndarray_complex_float_C, ndarray_complex_float_F)
_NDAC(std::complex<float>, ndarray_complex_float, ndarray_copy_complex_float_C, ndarray_copy_complex_float_F)
// This is the currently best option for this mess, tho if I can figure out a better one, I will banish this thing back to the place from whence it came
_NDAV(signed char, __pyx_fuse_0ndarray, __pyx_fuse_0ndarray_C, __pyx_fuse_0ndarray_F)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand the problem, I think maybe writing some doc to explain the bijectivity between array_type_t <-> dtype.
I think we need to write down that the order is really important

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could write this a bit more extreme in the code comment, but the thing won't compile if the order is wrong.
It would just complain, that the type is mismatched, but I totally support making this very very clear in the code comments.

@Maki4748
Copy link
Contributor Author

Maki4748 commented Dec 3, 2024

Thank for all the work, just some comments.

I Just merged a little patch to give the user the possibility to use his own version of eigen. So you can start your prototype for the tensors.

I looked it over yesterday, perfect, thanks, I saw this issue the other day, so it might be a while before we get any new Eigen version.

@Maki4748
Copy link
Contributor Author

Maki4748 commented Dec 3, 2024

@fbordeu Please look it over

@fbordeu fbordeu merged commit 7c691d7 into eigency-org:master Dec 4, 2024
45 checks passed
@Maki4748 Maki4748 deleted the typeFix branch December 4, 2024 16:29
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