Skip to content

Conversation

@Maki4748
Copy link
Contributor

  • Replaced deprecated pkg_resources (backwards compatible up to 3.7)
  • Added Macro in setup.py to force new numpy api
  • Replaced code belonging to old numpy api
  • Fixed Build Problems on Cython>=3.0.0 and Numpy>=2.0.0
  • Replaced np.npy_clongdouble with long double complex to fix compilation issues
  • Fixed dtypes that broke due to Numpy 2.0.0

@Maki4748
Copy link
Contributor Author

I wanted to do this in 2 different pull requests, but whatever now...

Basically, since Eigen added an experimental modul for Tensors, it might be a good time to change np ndarray=eigen 2darray for a possible implementation of the tensor module in the future.

I changed the names to ddarray, with one d for every dimension which fits nicely with a tensor implementation, because then we get the nd back for that. But yeah, at this point, more of an idea and possible option, than anything, but might still be good.

That being said:

The first commit is still a fix for everything broken right now, so maybe just ignore the second one if it's not wanted.

@fbordeu
Copy link
Contributor

fbordeu commented Nov 26, 2024

Hello, thanks for the interest on eigency. Can you explain why you change the user API from ndarray to ddarray. This breaks the API.

@Maki4748
Copy link
Contributor Author

Hello, thanks for the interest on eigency. Can you explain why you change the user API from ndarray to ddarray. This breaks the API.

To Preface this: I accidentally merged to main and it added it to the pull request, the only interesting pull is the first one, since that just makes it work for newer numpy and cython versions including the newer numpy api, which should be a default at this point.

Hi, I already sorta explained this in the comment above, but to explain this in a bit longer.
Numpy uses ndarray as a class and as a name, since it refers to the possible n dimensionality, while n is "only" 128 it is for most people too big to care about. But since it is still pretty normal to use Tensors > 2 dimensions, it's a bit misleading to have a function called ndarray which basically can't do more than two dimensions, but until recently Eigen couldn't do it either.

Now Eigen has introduced an experimental Module for Tensors in their dev branch and since right now Eigency isn't compatible with Cython>=3.0.0 and Numpy>=2.0.0, so code changes would need to happen in every code piece using it, it might be a good opportunity, to change the naming scheme up a bit and make it clearer what is meant, hence the dd for 2 dimensions, tho it could be argued that this could also just be decided in the ndarray function, tho then it might also be nice to only have the one with a flag for copy, view or none this is at this point all just me working in my dev branch on it. So please just focus on the first pull, I could also redo the pull if you want to. Also, third pull was me just removing a few lines of debugging code I forgot about, tho it is in the eigency_test module anyway and doesn't really matter right now.

@fbordeu
Copy link
Contributor

fbordeu commented Nov 28, 2024

Thanks for the clarifications. I will happily take the first commit with all the changes to update the library for numpy2 and cython=3.

Some comments/ideas:

I think imposing numpy>=2 is a little to hard, I will test it against numpy=1.* , to see if a can relax this constraint (some downstream codes are not numpy2 ready yet).
I think moving to a master of eigen is not a good idea. Master is a moving target, so master of today is not the master of tomorrow. I think the best way is to remove eigen from this repository and leave the user pull the desired version of eigen an do a local compilation if master is required.
Also I'm responsible of the eignecy package in conda-forge. In conda-forge only oficial releases (not master/devs/alphas) are accepted, this make the definition of the packages unique and non ambiguous.

I know eigen community has not make a release of eigen since 2021, I'm pushing them to make a release but is not an easy task.

I'm going work on the first part of you merge request. Then we can start working on how to expose the tensor part of eigne.

What do you think??

@fbordeu
Copy link
Contributor

fbordeu commented Nov 28, 2024

juste started a new merge request with your first commit and some extra changes.
#72

@Maki4748
Copy link
Contributor Author

Maki4748 commented Nov 28, 2024

Hey, I checked the merge request out and looks perfect.

The Numpy Version was only chosen by me cause of Python 3.13 anyway, having a build that also works on older Numpy versions makes it all the better.
Yeah, removing Eigen from the repo and bulding it against a local version if wanted is the better solution, we could put the tensor support behind a flag then, until there is a new Eigen with Tensors included.
I also didn't know about the conda constraints, but yeah, using a main version is out of the question then anyway.

I already started working on Tensor Support btw, tho that was like last night, so gonna take a bit longer.
There is another change I have in mind, but I'm gonna write a proposal about it in the coming days, it's about cleaning up conversions.pyx.

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