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 qiskit dependencies #51

Merged
merged 18 commits into from
Jun 16, 2022

Conversation

gcattan
Copy link
Collaborator

@gcattan gcattan commented Jun 13, 2022

The PR fixes #48, @toncho11 FYI
In addition to the refactoring of the QSVC into the SVC+quantum kernel, there was also a substantial amount of refactoring on the SPSA algorithm.


class1, class0 = self._split_classes(X, y)
y = self._map_classes_to_0_1(y)
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 is not a mistake. The _map_classes_to_0_1 is now taking place after the _split_classes.

Comment on lines -201 to +203
if self.quantum:
self._classifier.train(X, y, self._quantum_instance)
else:
self._classifier.train(X, y)
self._classifier.fit(X, y)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

QSVC now inherint from SVC, so they have the same fit, score and predict methods.


def _predict(self, X):
self._log("Prediction: ", X.shape)
print(self._training_input)
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 is probably a left over from a previous refactoring.

else:
classifier = SklearnSVM(self._training_input, gamma=self.gamma)
classifier = SVC(gamma=self.gamma)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Now that we use directly SVC, we can probably pass more than just gamma as an hyperparameter

training_size=30,
test_size=0,
n=feature_dim,
gap=0.3,
plot_data=False
)

X = np.concatenate((inputs['A'], inputs['B']))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The structure {inputs:{A:..., B:..} seems to have disappeared from qiskit.

@@ -166,7 +166,7 @@ def get_params(self):
# To achieve testing in a reasonnable amount of time,
# we will lower the size of the feature and the number of trials
return {
"n_samples": 4,
"n_samples": 6,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

error with vector dimension=4 did not give a clue on whether the problem is with the samples or the features.

@gcattan gcattan added the enhancement New feature or request label Jun 13, 2022
@gcattan gcattan self-assigned this Jun 13, 2022
@gcattan gcattan added this to the v0.0.2-dev milestone Jun 13, 2022
@gcattan gcattan marked this pull request as ready for review June 13, 2022 13:26
@gcattan gcattan changed the title Gc/update qiskit ml datasets Update qiskit dependencies Jun 14, 2022
@toncho11
Copy link
Collaborator

toncho11 commented Jun 16, 2022

One always needs to do that on Windows: How to fix “no lapack/blas resources found” on Windows

@gcattan
Copy link
Collaborator Author

gcattan commented Jun 16, 2022

Weird, I do not have this error on my local neither on the pipeline. Which test or example are you running ?

@toncho11
Copy link
Collaborator

toncho11 commented Jun 16, 2022

It works!!!

Using pip_search qiskit it says qiskit 0.36.2 18/05/2022

Bravo!

But for the instillation it is better to first execute:

pip install numpy pybind11 cython cmake osqp setuptools pytest matplotlib seaborn mne moabb

before python setup.py develop

Also it seems there is something wrong with scipy on Windows. It is a common problem. You may also need to install manually pyriemman from github to obtain pyriemman 0.2.8.dev0.

@toncho11
Copy link
Collaborator

Both tests and ERP moabb example work.

@gcattan
Copy link
Collaborator Author

gcattan commented Jun 16, 2022

Thanks for digging into this. I am so sorry, but I just remembered that the test requirements are listed as optional in the setup.py.
However, it is weird that the pyriemann version was not updated after running setup.py, and our ci/cd does not cover this use case. In this StackOverflow they suggest uninstalling the package before doing a clean install.
Nevertheless, it is a valid comment that we are missing some instructions on how to test and work around some of these drawbacks.

@gcattan gcattan merged commit 689b93a into pyRiemann:main Jun 16, 2022
@toncho11
Copy link
Collaborator

toncho11 commented Jun 16, 2022

I think these are missing from setup.py: numpy pybind11 cython cmake osqp setuptool. I had to manually install them and re-run
setup.py everytime.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update the version of Qiskit to 0.32
2 participants