Skip to content
This repository has been archived by the owner on Sep 13, 2023. It is now read-only.

Add numpy or refactor this #257

Closed
aguschin opened this issue May 18, 2022 · 3 comments · May be fixed by #290
Closed

Add numpy or refactor this #257

aguschin opened this issue May 18, 2022 · 3 comments · May be fixed by #290
Labels
bug Something isn't working

Comments

@aguschin
Copy link
Contributor

aguschin commented May 18, 2022

#221 introduced numpy dependency in api.py: https://github.com/iterative/mlem/blob/main/mlem/api/commands.py#L7
We either need to remove it, or we need to add numpy to install_requires in setup.py.

@aguschin aguschin added the bug Something isn't working label May 18, 2022
@aguschin
Copy link
Contributor Author

Right now MLEM fails if you install it with pip install mlem, cause numpy is not in install_requires.

mike0sv added a commit that referenced this issue May 19, 2022
@terryyylim
Copy link
Contributor

terryyylim commented May 20, 2022

Hey @aguschin @mike0sv , I just noticed this issue I introduced in my previous PR, sorry about that 🙏🏻 I thought through more deeply, and I think this merging of results should be dependent on different model types. What do y'all think about introducing a method on ModelType class, eg. merge_results, that take an array of a certain type and combine the results accordingly based on their types (e.g numpy array in the case of sklearn predict, predict_proba methods). In this case, commands.py will not need to know model-specific details.

@mike0sv
Copy link
Contributor

mike0sv commented May 21, 2022

Actually this should be a method of DatasetType, since the same model can output different types, but the idea is the same

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
3 participants