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

Update to Python-3.8 #254

Merged
merged 17 commits into from
Aug 11, 2023
Merged

Update to Python-3.8 #254

merged 17 commits into from
Aug 11, 2023

Conversation

qbarthelemy
Copy link
Member

@qbarthelemy qbarthelemy commented Jun 27, 2023

Update to Python-3.8

Update code to support SciPy 1.11.0 and scikit-learn 1.3.0.

Update readme for version 0.5, see #253.

@qbarthelemy
Copy link
Member Author

qbarthelemy commented Jun 27, 2023

scipy.stats.mode for non numeric data is deprecated from SciPy 1.9.0 and is removed in SciPy 1.11.0.

Tests are broken, because transfer learning module uses non numeric extended labels.
See encode_domains documentation: Note that if the classes were integers at first, they will be converted to strings.

@plcrodrigues , any idea?

@qbarthelemy
Copy link
Member Author

I solve the problem, replacing scipy.stats.mode by np.unique, which supports non numeric data.

@plcrodrigues
Copy link
Member

plcrodrigues commented Jun 28, 2023

@plcrodrigues , any idea?

This is indeed annoying but doesn't sound too hard to bypass with your np.unique approach (after corrections).

In any case, we should soon take some time to make use of the new sample-props feature from sklearn -- PR #24027 -- in the transfer learning module of pyRiemann. This would avoid recurring to string in the labels.

@@ -524,8 +523,8 @@ def predict(self, covtest):
"""
dist = self._predict_distances(covtest)
neighbors_classes = self.classmeans_[np.argsort(dist)]
out, _ = stats.mode(neighbors_classes[:, 0:self.n_neighbors], axis=1)
return out.ravel()
out = np.unique(neighbors_classes[:, 0:self.n_neighbors], axis=1)[:, 0]
Copy link
Member

Choose a reason for hiding this comment

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

I doubt this is correct

Copy link
Member

Choose a reason for hiding this comment

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

you would need to use return_counts and get the entry in out that has the biggest count

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right. Solution for ndarray is a little bit more complicated
https://stackoverflow.com/a/12300214

I will try to find the simplest solution for 2D case.

@@ -524,8 +523,8 @@ def predict(self, covtest):
"""
dist = self._predict_distances(covtest)
neighbors_classes = self.classmeans_[np.argsort(dist)]
out, _ = stats.mode(neighbors_classes[:, 0:self.n_neighbors], axis=1)
return out.ravel()
out = np.unique(neighbors_classes[:, 0:self.n_neighbors], axis=1)[:, 0]
Copy link
Member

Choose a reason for hiding this comment

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

I'm not completely sure that this will work all the time. The KNearestNeighbor is based on the idea of checking which class is the most common among the K nearest neighbors of the test point, right? Are we sure that np.unique always returns the unique classes of the neighbors in a sorted way? This example makes me doubt it...

Copy link
Member

Choose a reason for hiding this comment

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

You should probably use the output argument unique_counts from np.unique to be sure...

@qbarthelemy
Copy link
Member Author

qbarthelemy commented Jun 30, 2023

Last test for Windows Python 3.7 which is OK installed joblib 1.2.0
https://github.com/pyRiemann/pyRiemann/actions/runs/5321719371/jobs/9637189077#step:5:24
5:10

Now, it is KO installing joblib 1.3.1
https://github.com/pyRiemann/pyRiemann/actions/runs/5415226192/jobs/9846294273?pr=254#step:5:24

I try to freeze joblib to 1.2.0, but now, all others tests for Python 3.8 and 3.9 are KO. Any idea?

@qbarthelemy
Copy link
Member Author

qbarthelemy commented Jul 3, 2023

Last errors are due to scikit-learn 1.3.0, adding a new parameter sample_weight in KMeans ._init_centroids.
However, this parameter is not checked, raising an error when used with default value None.
scikit-learn/scikit-learn#26755

@qbarthelemy
Copy link
Member Author

Waiting scikit-learn bugfix, I decide to detect when _init_centroids must be used with a non default sample_weight.
I also clean code, removing support for version before scikit-learn 0.24.

@qbarthelemy qbarthelemy changed the title Update readme for release 0.5 Update code to support SciPy 1.11.0 and scikit-learn 1.3.0 Jul 3, 2023
@lesteve
Copy link

lesteve commented Jul 5, 2023

FYI I had a look and the joblib 1.3.1 issue is actually due to loky (default joblib parallel backend). The loky issue is tracked in joblib/loky#411.

Not entirely sure this is going to get fixed in loky. FYI scikit-learn requires Python 3.8+ since scikit-learn 1.1 (released May 2022), so a possible work-around for pyRiemann would be to drop Python 3.7 support in the 0.5 release.

@qbarthelemy qbarthelemy changed the title Update code to support SciPy 1.11.0 and scikit-learn 1.3.0 Update to Python-3.8 Jul 6, 2023
@qbarthelemy qbarthelemy closed this Aug 7, 2023
@qbarthelemy
Copy link
Member Author

Close-reopen for tests.

@qbarthelemy qbarthelemy reopened this Aug 7, 2023
@qbarthelemy
Copy link
Member Author

qbarthelemy commented Aug 7, 2023

Documentation fails, it is probably related to #255. @agramfort, what do you think ?

EDIT: no, it is not related

Copy link
Member

@agramfort agramfort left a comment

Choose a reason for hiding this comment

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

@qbarthelemy
Copy link
Member Author

Doc is now OK!
I let you merge @agramfort , @plcrodrigues , @sylvchev

@agramfort agramfort merged commit aa8fee7 into pyRiemann:master Aug 11, 2023
10 checks passed
@qbarthelemy qbarthelemy deleted the release_05 branch August 11, 2023 10:33
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