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

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

Open
wants to merge 1 commit into
base: 5.x
Choose a base branch
from

Conversation

musicinmybrain
Copy link

Copy link

@dpellegr dpellegr left a comment

Choose a reason for hiding this comment

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

Everything looks good from here.
We need this merged ASAP as these types have been dropped from Numpy.

@FooBarrior
Copy link

Hi all! Is this change going to be merged?

@orbingol
Copy link
Owner

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

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

orbingol commented Aug 11, 2023

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.

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.

5 participants