Skip to content

Conversation

@nathanjmcdougall
Copy link
Contributor

@nathanjmcdougall nathanjmcdougall commented Dec 12, 2025

scikit-learn is used in the pyemu.emulators module, among other places:

raise ImportError("LPFA emulator requires scikit-learn. Install with: pip install scikit-learn")
class GPR:
def __init__(self, *args, **kwargs):
raise ImportError("GPR emulator requires scikit-learn. Install with: pip install scikit-learn")
class StandardScalerTransformer:
def __init__(self, *args, **kwargs):
raise ImportError("StandardScalerTransformer requires scikit-learn. Install with: pip install scikit-learn")

graphviz is just used in misc\graph_pyemu.py.

@nathanjmcdougall nathanjmcdougall changed the title Declare scikit-learn as an optional dependency Declare scikit-learn and graphviz as optional dependencies Dec 12, 2025
@coveralls
Copy link

coveralls commented Dec 15, 2025

Coverage Status

coverage: 77.921% (+0.05%) from 77.871%
when pulling 50017c3 on nathanjmcdougall:fix/declare-scikit-learn-as-an-optional-dep
into 5dd3c78 on pypest:develop.

@briochh
Copy link
Collaborator

briochh commented Dec 15, 2025

my feeling is that the graph_pyemu isn't picked up anywhere. Let's check in with @aymanalz, we maybe be able to utilise it or drop graphviz as a dependancy.

@nathanjmcdougall
Copy link
Contributor Author

nathanjmcdougall commented Dec 15, 2025

Yeah I get the impression that the module there is intended for as a dev tool - to visualize the architecture of the modules.

I think it would be helpful to adopt dependency-groups from PEP 735 to give dedicated declaration sections for dev dependencies that aren't relevant for consumers of pyEMU as a package, but only for developers of pyEMU itself. Similarly, the test dependencies would probably be better as in a dependency-groups group than as optional dependencies.

This would allow us to declare lint dependencies etc. (e.g. if Ruff gets added) and so graphviz could be declared without adding it to the actual dependencies of the package. I'd put in a separate PR for that if you want it.

@briochh
Copy link
Collaborator

briochh commented Dec 15, 2025

I have a fork, that is slowly (and tentatively) exploring a move to uv for the whole project, and thus an updated (and more responsible pyproject.toml) https://github.com/briochh/pyemu/tree/feat_uv . perhaps we can bite the bullet on that at some point.

@nathanjmcdougall
Copy link
Contributor Author

That would be excellent. Probably in that case, we can hold off on declaring graphviz for now. I'll revert it when I get a moment.

@nathanjmcdougall nathanjmcdougall changed the title Declare scikit-learn and graphviz as optional dependencies Declare scikit-learn as an optional dependency Dec 16, 2025
@briochh briochh merged commit f9f5312 into pypest:develop Dec 16, 2025
14 checks passed
@aymanalz
Copy link
Collaborator

Thank you for checking about Graphviz dependency. I do not think I will use this package.

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.

4 participants