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

python3.10: numpy update and explicit int() #207

Closed
wants to merge 1 commit into from

Conversation

imyxh
Copy link
Contributor

@imyxh imyxh commented Jan 20, 2022

  • In Python 3.10, the Numpy C extensions refuse to build; this is fixed by setting the dependency to numpy>=1.21.1.
  • Implicit conversions to int are deprecated and enforced, as of Python 2.10. I think I have removed all implicit conversions from float to int, but I might've missed some, or done something in the wrong way. Review would be appreciated.

Fixes #204.
Fixes #206.

@imyxh imyxh changed the title python2.10: numpy update and explicit int() python3.10: numpy update and explicit int() Jan 20, 2022
Copy link
Owner

@tlecomte tlecomte left a comment

Choose a reason for hiding this comment

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

Thank you very much @imyxh ! This is very nice and helpful. I added a couple of comment about the Numpy version and some of the method return types. Once this is addressed, we can surely merge !

@@ -53,14 +53,14 @@ def include_dirs(self, dirs):
"sounddevice==0.4.2",
"rtmixer==0.1.3",
"docutils==0.17.1",
"numpy==1.21.1",
"numpy>=1.21.1",
Copy link
Owner

Choose a reason for hiding this comment

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

Could we pin to the precise version that's needed ?
I guess that 1.21.5 or 1.22.1 ?

In the future I might move to a different build tool that supports locking the whole dependency tree, but in the meantime I prefer to have pinned versions to avoid unexpected build failures when a package gets silently upgraded to a newer version.

"PyQt5==5.15.4",
"appdirs==1.4.4",
"pyrr==0.10.3",
]

# Cython and numpy are needed when running setup.py, to build extensions
setup_requires=["numpy==1.21.1", "Cython==0.29.24"]
setup_requires=["numpy>=1.21.1", "Cython==0.29.24"]
Copy link
Owner

Choose a reason for hiding this comment

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

Same comment as above, thanks !


@pyqtSlot(float, result=float)
@pyqtSlot(int, result=float)
Copy link
Owner

Choose a reason for hiding this comment

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

hmm, I think this one is not correct. The coordinates on the plot area are usually floats, they do not round nicely to integers.

@@ -58,25 +58,31 @@ def setBorders(self, start, end):
def setScale(self, scale):
self.scale = scale

@pyqtSlot(float, result=float)
@pyqtSlot(float, result=int)
Copy link
Owner

Choose a reason for hiding this comment

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

if it's possible, I think this method should keep returning floats. It might sound strange that screen coordinates would not be integers, but with display scaling this might not be that unusual

@imyxh
Copy link
Contributor Author

imyxh commented Jan 29, 2022

Okay, sounds good! I just pushed a new commit to my fork but for some reason it hasn't shown up here.... If it takes much longer I can just make a new PR.

@imyxh
Copy link
Contributor Author

imyxh commented Jan 30, 2022

Uhhhh okay I'm closing this one and submitting a new PR, someone else had the same issue and filed a bug here: community/community#10767

@imyxh imyxh closed this Jan 30, 2022
tlecomte added a commit that referenced this pull request Feb 13, 2022
python3.10: numpy update and explicit int() #207
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.

Python 3.10 support (building) Python 3.10 support (automatic float truncation)
2 participants