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

Fix numpy warnings #1473

Merged
merged 4 commits into from Jun 14, 2023
Merged

Fix numpy warnings #1473

merged 4 commits into from Jun 14, 2023

Conversation

mstimberg
Copy link
Member

This fixes a number of issues that will raise warnings in the forthcoming numpy version (1.25).

Copy link
Member

@thesamovar thesamovar left a comment

Choose a reason for hiding this comment

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

Can you briefly summarise what's going on here? I have to say I don't really understand the changes.

pass
if isinstance(value, str):
raise TypeError # Will be dealt with below
value = np.asanyarray(value).item()
Copy link
Member

Choose a reason for hiding this comment

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

Not sure I understand these changes, can you summarise?

Copy link
Member Author

Choose a reason for hiding this comment

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

This change is a slightly more complex issue related to what I wrote in my earlier comment: in future versions, we can no longer use float(x) as a check that x is either a scalar number or a length-1 array with a number (and not for example a string) – it will wail on length-1 arrays.

>>> assumptions = {'sin': DEFAULT_FUNCTIONS['sin'].pyfunc,
... 'pi': DEFAULT_CONSTANTS['pi'].value}
>>> assumptions = {'exp': DEFAULT_FUNCTIONS['exp'].pyfunc,
... 'inf': DEFAULT_CONSTANTS['inf'].value}
Copy link
Member

Choose a reason for hiding this comment

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

Any reason for change from sin to exp etc.? Not that it matters much, just curious.

Copy link
Member Author

Choose a reason for hiding this comment

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

I briefly explain this in the commit message: our docttest relied on the fact that np.sin(np.pi/2) == 1.0, which is reasonable but not guaranteed (results are only guaranteed to be < 4 epsilons away from the correct value). With the previous versions it matched 1.0 exactly, but with the newer, faster implementations in numpy 1.25 this is no longer the case (even with older versions, np.sin(0.0) or np.sin(np.pi) did not return 0.0). So I replaced it with exp(-inf) which I think is guaranteed to be exactly zero – at least it works with both numpy 1.24 and numpy 1.25 :)

@mstimberg
Copy link
Member Author

Can you briefly summarise what's going on here? I have to say I don't really understand the changes.

Sure. The main issue that is adressed here is that in a few places, we used code like this:

int(self.variables["N"].get_value())

The result of self.variables["N"].get_value() is sometimes a scalar value (e.g. for NeuronGroup where N is a constant), and sometimes a length-1 array (e.g. for Synapses, where the value can change). By wrapping it in int, we were sure about always getting a scalar Python int. Starting with numpy 1.25 (currently a release candidate), this will raise a warning, and in future versions it will raise an error. To replace this code, I added an item() function to ArrayVariable, Constant, and Quantity (the latter already had it inherited from numpy, but discarded the units). This mirrors the built-in function that numpy offers:

>>> import numpy as np
>>> np.array(3).item()
3
>>> np.array([3]).item()
3
>>> np.array([3, 4]).item()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: can only convert an array of size 1 to a Python scalar

@@ -357,7 +357,7 @@ def all_values(self):
"them."
)
indices = self.i[:]
sort_indices = np.argsort(indices)
Copy link
Member Author

Choose a reason for hiding this comment

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

BTW: This was always wrong (we need a stable sort here), but it only now got triggered with numpy 1.25 (and only on GitHub, not on my local machine).

Copy link
Member

@thesamovar thesamovar left a comment

Choose a reason for hiding this comment

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

OK, all that makes sense - thanks!

@mstimberg mstimberg merged commit 98a30dd into master Jun 14, 2023
46 checks passed
@mstimberg mstimberg deleted the fix_numpy_warnings branch June 14, 2023 15:23
mstimberg added a commit to brian-team/brian2cuda that referenced this pull request Jul 10, 2023
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.

None yet

2 participants