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

Bump scikit-learn #498

Merged
merged 1 commit into from
Jul 18, 2023
Merged

Conversation

danielhollas
Copy link
Contributor

scikit-learn build for version 0.24 started failing due to new version of Cython so we bump the version to 1.0. Per docs, this version does not bring any breaking changes. https://scikit-learn.org/1.0/auto_examples/release_highlights/plot_release_highlights_1_0_0.html

We also only use the sklearn.PCA in SMILES widget, I've verified that it still works and checked the release notes that there are not API changes to this method.
https://scikit-learn.org/1.0/whats_new/v1.0.html#changes-1-0

In the near future I'd like to get rid of scikit-learn altogether since it is a very heavy dependency and we only use it for PCA to reorient the molecules generated from SMILES by RDkit. I have a suspicion that this step is not really needed, and if it is, we can simply vendor the PCA function.

scikit-learn build for version 0.24 started failing
due to new version of Cython so we bump the version to 1.0.
Per docs, this version does not bring any breaking changes.
https://scikit-learn.org/1.0/auto_examples/release_highlights/plot_release_highlights_1_0_0.html

We also only use the sklearn.PCA in SMILES widget,
I've verified that it still works and checked the release notes
that there are not API changes to this method.
https://scikit-learn.org/1.0/whats_new/v1.0.html#changes-1-0
@codecov
Copy link

codecov bot commented Jul 18, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (d644768) 79.63% compared to head (741e15c) 79.63%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #498   +/-   ##
=======================================
  Coverage   79.63%   79.63%           
=======================================
  Files          27       27           
  Lines        3752     3752           
=======================================
  Hits         2988     2988           
  Misses        764      764           
Flag Coverage Δ
python-3.10 79.63% <ø> (ø)
python-3.8 79.67% <ø> (?)
python-3.9 79.67% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@unkcpz unkcpz left a comment

Choose a reason for hiding this comment

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

Look good to me.

@danielhollas
Copy link
Contributor Author

@unkcpz thanks! I'll merge this to unblock the other PRs.

@danielhollas danielhollas merged commit 3f15948 into aiidalab:master Jul 18, 2023
7 checks passed
@danielhollas danielhollas deleted the bump-scikitlearn branch July 18, 2023 15:25
@yakutovicha
Copy link
Member

We should have probably put scikit-learn~1.0 to have more freedom on the versions

@danielhollas
Copy link
Contributor Author

I've done this on purpose to make sure things work for now. As explained in the OP I want to get rid of it altogether soon.

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