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

COMPAT: ensure we pass numpy array to cKDtree and combat with sklearn 1.3.0 #107

Merged
merged 3 commits into from
Jul 2, 2023

Conversation

martinfleis
Copy link
Member

@martinfleis martinfleis commented Jun 30, 2023

It seems that the DataFrame input to query worked but was never actually supported. This ensures we always cast it to a numpy array.

Closes #106

We should probably cut a patch release with this as any env with new scipy will have this issue.

@martinfleis martinfleis requested a review from jGaboardi June 30, 2023 21:48
@martinfleis
Copy link
Member Author

mmm. New failures are caused by fresh scikit-learn 1.3.0 where KDTree.valid_metrics is no longer an attribute but a method. Will look into that.

@martinfleis
Copy link
Member Author

Apparently KDtree.valid_metrics was not considered public before? And they now added KDtree.valid_metrics() in scikit-learn/scikit-learn#25482 See scikit-learn/scikit-learn#26742

@martinfleis martinfleis changed the title COMPAT: ensure we pass numpy array to cKDtree COMPAT: ensure we pass numpy array to cKDtree and combat with sklearn 1.3.0 Jun 30, 2023
@codecov
Copy link

codecov bot commented Jun 30, 2023

Codecov Report

Merging #107 (8d616ca) into main (44ce06b) will increase coverage by 0.18%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #107      +/-   ##
==========================================
+ Coverage   52.38%   52.56%   +0.18%     
==========================================
  Files          12       12              
  Lines        1844     1851       +7     
  Branches      316      317       +1     
==========================================
+ Hits          966      973       +7     
  Misses        821      821              
  Partials       57       57              
Impacted Files Coverage Δ
pointpats/geometry.py 78.57% <100.00%> (+0.93%) ⬆️
pointpats/pointpattern.py 70.05% <100.00%> (ø)

@martinfleis martinfleis merged commit 308f0df into pysal:main Jul 2, 2023
@martinfleis martinfleis deleted the kdtree_fix branch July 2, 2023 20:56
@martinfleis
Copy link
Member Author

sorry for a self-merge but this was completely blocking any testing now and I wanted to work on #109 and #108

@martinfleis
Copy link
Member Author

Note to myself that we may need to eventually revert the sklearn compat part because it is a regression in upstream and they will restore the original behaviour (property, not a method).


if metric in KDTree.valid_metrics:
if Version(sklearn.__version__) >= Version("1.3.0"):
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 will need to be

if Version(sklearn.__version__) == Version("1.3.0"):

The change was reverted in upstream after my report.

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.

CI: TypeError: float() argument must be a string or a number, not 'PointPattern'
1 participant