This repository has been archived by the owner on Jun 12, 2023. It is now read-only.
Make scikit-learn an optional dependency #524
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We previously had scikit-learn as a requirement that always needed to be installed. However it was only ever used by the measurement discriminators and not needed for ignis in general. Since this is a fairly large package it is a bit heavy to require for every install especially if the discriminators aren't being used. This commit removes scikit-learn from the requirements list and adds it as an optional dependency.
|
Perhaps in the future we would like to use ML tools for advanced experiment analysis |
|
Well we can leave it as a hard requirement. I was under the assumption that sklearn was only used by the discriminators and since that's not widely used by all ignis users it felt like a better fit as an optional dependency since scikit learn is large package. But, if there are plans to add more scikit-learn usage to the rest of ignis it probably doesn't make sense to do this then |
|
@chriseclectic - what is your opinion? |
chriseclectic
approved these changes
Mar 29, 2021
mtreinish
added
the
Changelog: API Change
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
We previously had scikit-learn as a requirement that always needed to be
installed. However it was only ever used by the measurement
discriminators and not needed for ignis in general. Since this is a
fairly large package it is a bit heavy to require for every install
especially if the discriminators aren't being used. This commit removes
scikit-learn from the requirements list and adds it as an optional
dependency.
Details and comments