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

Feat/mdm predict distances #248

Merged
merged 26 commits into from
Mar 5, 2024

Conversation

gcattan
Copy link
Collaborator

@gcattan gcattan commented Feb 29, 2024

This PR:

  • moves the workaround _predict_distances from the distance to the classification module.
  • changes the workaround, so that we do not override the _predict_distances of the MDM class but only the one of a particular instance.
  • separate computation of distances from weights
  • rename dist_logeuclid_cpm to dist_logeuclid_to_convex_hull_cpm
  • add toy functions euclid_cpm and logeuclid_cpm (the idea would be rather to re-implement the recursion in the pyRiemann distance module in the CPM way)
  • add warnings concerning the use of CPM distances with MDM.

@@ -472,7 +472,7 @@ def __init__(

def _create_pipe(self):
clf_mean_logeuclid_dist_cpm = QuantumMDMWithRiemannianPipeline(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This VotingClassifier should be replaced by a voting classifier Q-MDM/NHC or Q-MDM/MDM

Comment on lines 172 to 178
objectif = prob.sum(
w[i] * distance(B, A[i]) for i in matrices
)

prob.set_objective("min", objectif)
prob.add_constraint(prob.sum(w) == 1)
weights = optimizer.solve(prob, reshape=False)
Copy link
Collaborator Author

@gcattan gcattan Feb 29, 2024

Choose a reason for hiding this comment

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

This is pretty basic. We can think of something clever in the future. Let's just put the structure in place.

@toncho11
Copy link
Collaborator

Can you please put a more detailed description of the PR.

@@ -16,6 +17,7 @@
NaiveQAOAOptimizer,
set_global_optimizer,
)
from pyriemann_qiskit.utils.distance import distance_functions, is_cpm_dist
Copy link
Member

Choose a reason for hiding this comment

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

is_cpm_dist should be moved from utils.distance to classification.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hm. Yes, but I will need to move is_qmean too, but it is also used in the pipeline module.

Copy link
Member

Choose a reason for hiding this comment

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

We should merge is_qdist and is_qmean into a generic is_qfunction.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok. Let's create a new module utils/utils.py?

pyriemann_qiskit/utils/distance.py Outdated Show resolved Hide resolved
pyriemann_qiskit/utils/distance.py Outdated Show resolved Hide resolved
pyriemann_qiskit/utils/distance.py Outdated Show resolved Hide resolved
gcattan and others added 8 commits March 1, 2024 13:32
pyriemann_qiskit/classification.py Outdated Show resolved Hide resolved
pyriemann_qiskit/classification.py Outdated Show resolved Hide resolved
pyriemann_qiskit/utils/mean.py Show resolved Hide resolved
pyriemann_qiskit/utils/mean.py Show resolved Hide resolved
pyriemann_qiskit/utils/mean.py Outdated Show resolved Hide resolved
pyriemann_qiskit/utils/distance.py Outdated Show resolved Hide resolved
pyriemann_qiskit/utils/distance.py Outdated Show resolved Hide resolved
pyriemann_qiskit/utils/distance.py Outdated Show resolved Hide resolved
pyriemann_qiskit/classification.py Outdated Show resolved Hide resolved
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.

Thx @gcattan !
Last remark: don't forget to update whatsnew file.

@gcattan
Copy link
Collaborator Author

gcattan commented Mar 4, 2024

Thx @qbarthelemy :)

Until now I was updating the whatsnew on each release only, based on the generated release notes.
Do you mean to change this?

@qbarthelemy
Copy link
Member

Do you mean to change this?

No, don't change anything.

@gcattan gcattan marked this pull request as ready for review March 5, 2024 05:55
@gcattan gcattan merged commit a89478b into pyRiemann:main Mar 5, 2024
13 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.

3 participants