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

Implementation of NCH (Nearest Convex Hull) classifier #253

Merged
merged 47 commits into from
Mar 18, 2024

Conversation

toncho11
Copy link
Collaborator

@toncho11 toncho11 commented Mar 5, 2024

QuanticNCH uses NearestConvexHull classifier (non quantum version)
Resolves #246
Resolves #254

@toncho11 toncho11 marked this pull request as draft March 5, 2024 16:13
Copy link
Member

@qbarthelemy qbarthelemy left a comment

Choose a reason for hiding this comment

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

I suggest a simpler implementation for NCH, and directly multiclass.

pyriemann_qiskit/classification.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@gcattan gcattan left a comment

Choose a reason for hiding this comment

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

Good job @toncho11!
I see you introduced the n_hulls and n_samples_per_hull parameters.
We could do some hyperparmetrization in a follow-up PR.

I see that the signature of the distance function differs a little bit from pyRiemann, and this is why you could not reuse the whole suggestion from @qbarthelemy in the _predict_distances method, but you introduced the transform.

Some minor comments. Questions:

  1. Do we keep the classifly_P300_nch.py? (I think it could be handfull for the next iterations)
  2. Do we want to do a short test with quantum=True before merging?

pyriemann_qiskit/utils/distance.py Outdated Show resolved Hide resolved
pyriemann_qiskit/classification.py Outdated Show resolved Hide resolved
pyriemann_qiskit/classification.py Outdated Show resolved Hide resolved
pyriemann_qiskit/classification.py Show resolved Hide resolved
pyriemann_qiskit/classification.py Outdated Show resolved Hide resolved
pyriemann_qiskit/classification.py Outdated Show resolved Hide resolved
pyriemann_qiskit/classification.py Outdated Show resolved Hide resolved
examples/ERP/classify_P300_nch_no_moabb.py Outdated Show resolved Hide resolved
@toncho11
Copy link
Collaborator Author

I think the PR is ready for merge.

pyriemann_qiskit/classification.py Show resolved Hide resolved
pyriemann_qiskit/classification.py Outdated Show resolved Hide resolved
pyriemann_qiskit/classification.py Outdated Show resolved Hide resolved
pyriemann_qiskit/classification.py Outdated Show resolved Hide resolved
pyriemann_qiskit/classification.py Outdated Show resolved Hide resolved
pyriemann_qiskit/classification.py Outdated Show resolved Hide resolved
toncho11 and others added 5 commits March 12, 2024 19:53
Set of SPD matrices.

Co-authored-by: Quentin Barthélemy <q.barthelemy@gmail.com>
Added new lines to before Parameters

Co-authored-by: Quentin Barthélemy <q.barthelemy@gmail.com>
[y == c, :, :] => [y == c]

Co-authored-by: Quentin Barthélemy <q.barthelemy@gmail.com>
NearestConvexHull text change

Co-authored-by: Quentin Barthélemy <q.barthelemy@gmail.com>
@pyRiemann pyRiemann deleted a comment from qbarthelemy Mar 13, 2024
@toncho11 toncho11 dismissed qbarthelemy’s stale review March 13, 2024 17:54

Changes already applied.

@toncho11 toncho11 self-assigned this Mar 13, 2024
toncho11 and others added 2 commits March 15, 2024 08:59
Added support for both "min-hull" and "random-hull" using the constructor parameter "hull-type".
Copy link
Collaborator

@gcattan gcattan left a comment

Choose a reason for hiding this comment

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

Ok for me.

@toncho11 toncho11 merged commit a0241ce into pyRiemann:main Mar 18, 2024
13 checks passed
Copy link
Member

@qbarthelemy qbarthelemy left a comment

Choose a reason for hiding this comment

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

Better late than never.

Comment on lines +943 to +946
predictions = [
self.classes_[min(range(len(values)), key=values.__getitem__)]
for values in dist
]
Copy link
Member

Choose a reason for hiding this comment

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

What is the difference with predictions = self.classes_[dist.argmin(axis=1)] ?

for test_sample in X:
dist_sample = self._process_sample(test_sample)
dist.append(dist_sample)

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 add dist = np.concatenate(dist, axis=1) here, in order to transform list into array.

Comment on lines +841 to +844
distances_to_covs = [
distance_logeuclid(test_sample, cov)
for cov in self.matrices_per_class_[c]
]
Copy link
Member

Choose a reason for hiding this comment

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

Using from pyriemann.utils.distance import distance, you can simplify code this way
distances_to_covs = distance(self.matrices_per_class_[c], test_sample, metric="logeuclid")[:, 0]

Then, you can compute np.argsort(distances_to_covs) instead of np.argsort(np.array(distances_to_covs)).

@qbarthelemy qbarthelemy mentioned this pull request Mar 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants