search

orbingol / NURBS-Python

Object-oriented pure Python B-Spline and NURBS library
https://onurraufbingol.com/NURBS-Python/
MIT License
636 stars 156 forks source link

Stop using deprecated/removed np.float/np.int #163

Open musicinmybrain opened 1 year ago

musicinmybrain commented 1 year ago

See: https://numpy.org/devdocs/release/1.20.0-notes.html#deprecations

FooBarrior commented 1 year ago

Hi all! Is this change going to be merged?

orbingol commented 1 year ago

There are some other changes that need to be merged, such as Matplotlib's and some others, but v5.x version still supports the EOL Python v2.7. There will be a v6.0 which would eventually move to the mainstream supported versions of Python. The point is that the core geomdl library won't need any change, it has the least amount of dependencies and it might only break if Python standard library changes.

As of last week, I was thinking that nobody would use Python v2.7 anymore and I ended up compiling some of our research code to validate something unrelated to NURBS-Python. This also justifies to me that in the research industry, Python v2.7 could still be used. Of course, it shouldn't be used, but we could say the same things for several other things.

Another thing is that the core geomdl library is not dependent of numpy. Visualizers are always though to be "extra", think it as an internal request to make the life "temporarily" easy. Although one would be more than enough, that's why there are three different visualizers.

Due to the design principle as mentioned, the visualizer package can be moved outside of geomdl pretty easily. I remember from other users customizing existing ones and creating new ones with very little effort. You are not limited to some specific version of MPL, Plotly or VTK. You can even create a separate package with the new visualizers on PyPI, I'd really appreciate it if you do and that was the initial plan for this.

To answer @FooBarrior question, I am afraid to say that v5.x needs to stay this way. However, like mentioned above, there are several other solutions and I think they are easy to implement.

musicinmybrain commented 1 year ago

To answer @FooBarrior question, I am afraid to say that v5.x needs to stay this way. However, like mentioned above, there are several other solutions and I think they are easy to implement.

Does this PR not work on Python 2.7? I would have thought that replacing np.int with int and np.float with np.float64 would be backwards-compatible with any Python and Numpy versions.

I understand that Python 2.7 is limiting, and that this does not touch the “core,” but I’m not sure I understand why it shouldn’t be fixed. It seems like it would be easy to work around any compatibility problems that might exist in this PR rather than waiting on architectural changes and a new major version.

orbingol commented 1 year ago

Does this PR not work on Python 2.7? I would have thought that replacing np.int with int and np.float with np.float64 would be backwards-compatible with any Python and Numpy versions.

I believe numpy dropped support for Python v2.7 years ago. It makes sense as Py2 is EOL. The backwards compatibility needs to be investigated and that might also limit the minimum version of numpy used.

I understand that Python 2.7 is limiting, and that this does not touch the “core,” but I’m not sure I understand why it shouldn’t be fixed. It seems like it would be easy to work around any compatibility problems that might exist in this PR rather than waiting on architectural changes and a new major version.

This PR only fixes one single issue, but there will be more issues to fix due to updates in the dependent libraries and that's going to take some time to test. I have to update the automated CI checks for sure, I am guessing some of the hooks are gone. It comes to time and it is a more limiting factor to me nowadays. As I mentioned in my previous comment, visualizers are easy to modify and extend as they are not part of the core. There is also an option to use the older version of numpy just for a while, if your code is not dependent on the latest updates.

From the maintenance perspective, it is not a good idea to change dependencies or minimum requirements with the minor or patch level upgrades. It is a very annoying change when you are on a strict deadline or very busy to follow the release notes. It makes sense to create an intermediate version, say v5.5.x which would come with some compatibility updates. Although it looks like a minor version upgrade, that's usually common notation in software indicating a compatibility fix.