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.

Sign up for GitHub

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

Jump to bottom

Exposure of Gamma, optimizer and variational form #16

Merged
merged 32 commits into from
Dec 3, 2021
Merged

Exposure of Gamma, optimizer and variational form #16

merged 32 commits into from
Dec 3, 2021

Conversation

gcattan
Copy link
Collaborator

@gcattan gcattan commented Nov 30, 2021

#13

qbarthelemy
qbarthelemy requested changes Dec 1, 2021
View reviewed changes
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.

Good job! Don't hesitate to have a look to new tests of pyRiemann, to write your tests with a systematic checking of all parameters and functions.

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/utils/hyper_params_factory.py Outdated Show resolved Hide resolved
pyriemann_qiskit/utils/hyper_params_factory.py Outdated Show resolved Hide resolved
gcattan and others added 5 commits December 1, 2021 20:11
@gcattan @qbarthelemy
Update pyriemann_qiskit/classification.py
f1617f9 
Co-authored-by: Quentin Barthélemy <q.barthelemy@gmail.com>
@gcattan @qbarthelemy
Update pyriemann_qiskit/classification.py
de212f1 
Co-authored-by: Quentin Barthélemy <q.barthelemy@gmail.com>
@gcattan @qbarthelemy
Update pyriemann_qiskit/classification.py
01a6f7d 
Co-authored-by: Quentin Barthélemy <q.barthelemy@gmail.com>
@gcattan @qbarthelemy
Update pyriemann_qiskit/classification.py
979faaa 
Co-authored-by: Quentin Barthélemy <q.barthelemy@gmail.com>
@gcattan @qbarthelemy
Update pyriemann_qiskit/utils/hyper_params_factory.py
a8a5d79 
Co-authored-by: Quentin Barthélemy <q.barthelemy@gmail.com>
@gcattan
Copy link
Collaborator Author

gcattan commented Dec 1, 2021

Thank you for the feedback and the suggestions!
I have tried to apply @pytest.mark.parametrize as much as possible and I plan to come back to test_classification.py when I will be working on #11.
But I can of course address some of the refactorings here if you would prefer.

@gcattan gcattan marked this pull request as ready for review December 1, 2021 19:43
@sylvchev
Copy link
Member

sylvchev commented Dec 3, 2021

Good for me!
We could merge this and you could update test_classification.py in next PR. Ok for you?

@gcattan
Copy link
Collaborator Author

gcattan commented Dec 3, 2021

Ok for me :)

qbarthelemy
qbarthelemy approved these changes Dec 3, 2021
View reviewed changes
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.

Ok to update test_classification.py in a next PR.

@qbarthelemy qbarthelemy merged commit 769605b into pyRiemann:main Dec 3, 2021
@qbarthelemy
Copy link
Member

Thx @gcattan

@gcattan gcattan deleted the gc/hyperparams2 branch December 3, 2021 19:22
@gcattan
Copy link
Collaborator Author

gcattan commented Dec 3, 2021

Thank you both for the time reviewing the PRs :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qbarthelemy qbarthelemy qbarthelemy approved these changes

@sylvchev sylvchev sylvchev approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gcattan @sylvchev @qbarthelemy