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

Shots and feature map exposure #14

Merged
merged 42 commits into from
Nov 25, 2021
Merged

Conversation

gcattan
Copy link
Collaborator

@gcattan gcattan commented Nov 19, 2021

Expose the number of shots and the feature map (part of issue #13)

@gcattan gcattan marked this pull request as ready for review November 19, 2021 21:29
Copy link
Member

@sylvchev sylvchev left a comment

Choose a reason for hiding this comment

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

Nice work!

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.

LGTM! Only minor comments.

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
pyriemann_qiskit/classification.py Outdated Show resolved Hide resolved
@gcattan
Copy link
Collaborator Author

gcattan commented Nov 23, 2021

Thank you for the comments. There is still some problem with the pipeline. I should investigate it deeper, but it seems that git failed to be installed on the doc pipeline. If you have an idea do not hesitate :)

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.

Again, some minor remarks.

More generally:

1 - don't merge master in your branch, but rebase your branch on master; otherwise, it is really difficult to understand if modifications com from branch and from master.

2 - to easily update your fork master on the pyRiemann-qiskit master, do:
git remote add upstream https://github.com/pyriemann/pyriemann-qiskit.git
Then, before creating a branch, pull from upstream.

See SciPy guidelines: https://docs.scipy.org/doc/scipy/reference/dev/contributor/development_workflow.html

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
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
@qbarthelemy
Copy link
Member

qbarthelemy commented Nov 23, 2021

There is still some problem with the pipeline. I should investigate it deeper, but it seems that git failed to be installed on the doc pipeline. If you have an idea do not hesitate :)

I think that the problem comes from the new module datasets of pyRiemann, not declared in init file. Its import fails when we are not in pyRiemann package. This should be corrected after merge of
pyRiemann/pyRiemann@91c62e1
@sylvchev

gcattan and others added 11 commits November 24, 2021 09:19
Co-authored-by: Quentin Barthélemy <q.barthelemy@gmail.com>
Co-authored-by: Quentin Barthélemy <q.barthelemy@gmail.com>
Co-authored-by: Quentin Barthélemy <q.barthelemy@gmail.com>
Co-authored-by: Quentin Barthélemy <q.barthelemy@gmail.com>
Co-authored-by: Quentin Barthélemy <q.barthelemy@gmail.com>
Co-authored-by: Quentin Barthélemy <q.barthelemy@gmail.com>
- correct and improve description of gen_zz_feature_map
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.

Great job!
Last comments, then it will be ok for me.

tests/test_utils_hyper_params_factory.py Outdated Show resolved Hide resolved
tests/conftest.py Outdated Show resolved Hide resolved
@gcattan
Copy link
Collaborator Author

gcattan commented Nov 25, 2021

Thank you for the useful comments! There is still some CI/CD error. I will wait for the aforementioned commit to be merged and come back to this if required :)

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 !

@qbarthelemy qbarthelemy merged commit f945753 into pyRiemann:main Nov 25, 2021
@gcattan gcattan deleted the gc/hyperparams branch December 3, 2021 19:22
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