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

replace np.int with np.int64 #1017

Merged
merged 2 commits into from
Oct 4, 2023
Merged

Conversation

cycl0n3sab3r
Copy link
Contributor

np.int deprecated in numpy 1.20 and removed in numpy 1.24, suggest replacing with np.int64

`np.int` has been deprecated, suggest changing to `np.int64`
@codecov
Copy link

codecov bot commented May 25, 2023

Codecov Report

Merging #1017 (5aae3cf) into devel (9ec3e50) will not change coverage.
The diff coverage is 100.00%.

Additional details and impacted files
@@           Coverage Diff           @@
##            devel    #1017   +/-   ##
=======================================
  Coverage   54.49%   54.49%           
=======================================
  Files         210      210           
  Lines       21560    21560           
  Branches     3169     3169           
=======================================
  Hits        11750    11750           
  Misses       9255     9255           
  Partials      555      555           

@bpkroth
Copy link

bpkroth commented Jun 28, 2023

@zhenwendai @MartinBubel, you're some of the last commit authors in the history. Any chance we can get this committed and a new version cut soon? There are several projects that rely on it that are currently having issues with recently Python and numpy version mismatches.

@MartinBubel
Copy link
Contributor

Thanks for picking this up @cycl0n3sab3r .

Regarding the deprecations site, np.int was an alias for the native python int. Thus, I would suggest to not use np.int64 but instead use native python int. We did this in a previous MR replacing np.bool by bool.

I think this is more "save" as np.int64 is not listed in the lastet datatypes list of numpy. I think it would be best to stick with native python types whenever possible and using the "conversion to c types" only at the places where it is necessary.

Also, there are a lot more usages of np.int than the one in your commit. Could you please update them as well @cycl0n3sab3r ?

Furthermore, this is not the first PR concerning deprecated numpy types. Could you please check the usage of any of the deprecated types in this table and replace them as recommended at the above table?

That would be very helpful. I could also do it but I think its best if you update your PR.

@bpkroth I won't be allowed to merge. Maybe @zhenwendai can help.

@ekalosak
Copy link
Contributor

I'm in favor of incremental fixes, personally - no need to saddle the contributor with the extra work for code paths that are either not covered by testing or not frequently used. IMHO.

That said, Martin, your point on using int rather than np.int64 strikes me as correct. But wouldn't it be better to make the int subtype configurable with the base int as a default? Polymorphism with good defaults seems even better than swapping the hardcoded type, no?

@MartinBubel
Copy link
Contributor

Configurable type with base int sounds legit @ekalosak 👍 Although, I am bit scared of possible side effects of subsequent calls as the first comment in the function goes

This is quite complex and will take a while to understand,
    so do not change anything in here lightly!!!

But so far I could not find anything suspicious on the type yet.

@ekalosak
Copy link
Contributor

ekalosak commented Sep 24, 2023

CI says ok, let's merge. If we get bug reports on the new version we can revert and release anew.
And we we will have learned about the test gaps. These test gaps are good first issues: nicely scoped and quickly productive.

We also need to release to PyPi pretty ASAP... it's been a while.
https://github.com/SheffieldML/GPy/wiki/Release-process is straightforward. Probably should be weekly automated IMHO: #1033

@MartinBubel MartinBubel merged commit 279f8e8 into SheffieldML:devel Oct 4, 2023
4 checks passed
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

4 participants