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

FutureWarning issued by scikit-learn in opflow #7097

Closed
jakelishman opened this issue Oct 4, 2021 · 16 comments · Fixed by #8299
Closed

FutureWarning issued by scikit-learn in opflow #7097

jakelishman opened this issue Oct 4, 2021 · 16 comments · Fixed by #8299
Labels
bug Something isn't working good first issue Good for newcomers mod: opflow Related to the Opflow module

Comments

@jakelishman
Copy link
Member

Information

  • Qiskit Terra version: 0.18+
  • Python version: any
  • Operating system: any

What is the current behavior?

With the new(ish) scikit-learn version 1.0, there is a FutureWarning issued when using the normalize parameter of LinearModel, which opflow does when calculating gradients. This doesn't fail our tests (though perhaps it should), because FutureWarning doesn't count as a type of DeprecationWarning.

Steps to reproduce the problem

Run the test suite, especially test.python.opflow.test_gradients.TestGradients.test_natural_gradient.

What is the expected behavior?

No future warning.

Suggested solutions

Fix opflow to handle fitting with StandardScaler rather than the now-deprecated normalize parameter. This has existed for several minor releases of scikit-learn, so should be safe for us to do.

@jakelishman jakelishman added the bug Something isn't working label Oct 4, 2021
@woodsp-ibm woodsp-ibm added the mod: opflow Related to the Opflow module label Oct 4, 2021
@jakelishman jakelishman added the good first issue Good for newcomers label Oct 6, 2021
@st4eve
Copy link

st4eve commented Oct 6, 2021

Hi, I would like to work on this as my first issue. If someone wouldn't mind assigning me that would be great. Thanks!

@jakelishman
Copy link
Member Author

No problem, I'll assign it to you.

@levbishop
Copy link
Member

See also #6904 where I suggest erroring on all (unexpected) warnings.

@jakelishman
Copy link
Member Author

Yeah, absolutely agreed on that. It's something I want too - I had started to work on reducing the number of allowed deprecation warnings as a precursor to this, but then other stuff took over.

@jakelishman
Copy link
Member Author

Hi @st4eve, how's it going? Do you need any help on getting this working?

@st4eve
Copy link

st4eve commented Nov 11, 2021 via email

@jakelishman
Copy link
Member Author

We would probably need to see the full logs that those commands allude to, but most likely the problem is that you don't have a correctly working C compiler on your system. There's a little intro to the "building Terra from source" section on the website that says:

On Windows, it is easiest to install the Visual C++ compiler from the Build Tools for Visual Studio 2017. You can instead install Visual Studio version 2015 or 2017, making sure to select the options for installing the C++ compiler.

Have you got those installed? In particular, since that link seems to be slightly out of date now, you want the thing that's now marked "Build Tools for Visual Studio 2022", and if it ever gives you an option (I don't think it does), make sure that you install the C++ tools.

@javabster
Copy link
Contributor

I'd also recommend double checking you've followed all the steps in the Installing terra from source, including the final pip install -e . step 😄

@1ucian0
Copy link
Member

1ucian0 commented Jan 31, 2022

How is that going @st4eve ? I also would recommend you to ask for help in the channel #qiskit-pr-help in our slack workspace: https://join.slack.com/share/enQtMzAzNDA0MDQzMDMyMy0yZWY4MGU4NDJmM2UzYjc1YjQ4OTg0YmY3Y2FlYzgzMmVhMDU2MDgwMDM5MjM0ZDQyYTA2YjA2OWIzMDc5MDEz

@Cryoris
Copy link
Contributor

Cryoris commented Feb 12, 2022

Is this issue actually still present? I just ran the opflow tests and did not encounter any FutureWarning anymore. 🙂

@jakelishman
Copy link
Member Author

@prakharb10
Copy link
Contributor

Hi, there is a difference between the deprecated normalize parameter and the StandardScaler outlined here Will that be of any concern, maybe in the tests?

@jakelishman
Copy link
Member Author

@Cryoris, @woodsp-ibm: perhaps you know the answer to this question?

@prakharb10
Copy link
Contributor

Hi, following up on this if there's any updates?

@Zoufalc
Copy link
Contributor

Zoufalc commented Jun 30, 2022

Since the normalize argument is only used in an empirical regularization method, i.e., Ridge, it should be perfectly fine to implement minor changes such as changing to StandardScaler.

@jakelishman
Copy link
Member Author

Thanks, Christa. @prakharb10: the comment above is from the person who originally wrote the code, so we can go ahead with that. Since it may affect the exact results achieved from a given seed / set of initial points, we should include a one-line "upgrade" release note with the change mentioning that, but it should be sufficiently internal that we don't need any deprecation cycles, I think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers mod: opflow Related to the Opflow module
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

9 participants