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

[DEE-456] nlp dummy model refactoring #2511

Merged
merged 14 commits into from May 11, 2023

Conversation

yromanyshyn
Copy link
Contributor

@Nadav-Barak

Currently we have a dummy model for tabular and another one for nlp but they basically do the same stuff.
We would like them to have common abstract containing most of the logic with child class implement datadype specific functionalities.

they do not have much in common, in general, _predict and _predict_proba methods could be separated into some kind of base class, but in this case, we would need to unify nlp.TextData and tabular.Dataset APIs, everything else is just a validation logic that is pretty specific IMO. So, I do not see a need in creating some kind of ambiguous and not clear abstractions just to reduce a few lines of code

In general, I just refactored the nlp dummy model and moved the validation logic into the input_validation.py module

@yromanyshyn yromanyshyn added the refactoring Making significant changes to structure of code label May 10, 2023
@yromanyshyn yromanyshyn self-assigned this May 10, 2023
@yromanyshyn yromanyshyn requested a review from a team as a code owner May 10, 2023 05:58
deepchecks/nlp/context.py Outdated Show resolved Hide resolved
deepchecks/nlp/context.py Outdated Show resolved Hide resolved
deepchecks/nlp/context.py Show resolved Hide resolved
deepchecks/nlp/context.py Outdated Show resolved Hide resolved
deepchecks/nlp/context.py Outdated Show resolved Hide resolved
deepchecks/nlp/input_validations.py Show resolved Hide resolved
deepchecks/nlp/input_validations.py Show resolved Hide resolved
@yromanyshyn yromanyshyn requested a review from noamzbr May 10, 2023 10:27
@noamzbr noamzbr enabled auto-merge (squash) May 10, 2023 12:57
Copy link
Collaborator

@Nadav-Barak Nadav-Barak left a comment

Choose a reason for hiding this comment

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

To verify, either way predictions and probabilities return as np array?

deepchecks/nlp/input_validations.py Show resolved Hide resolved
deepchecks/nlp/input_validations.py Outdated Show resolved Hide resolved
@yromanyshyn
Copy link
Contributor Author

@Nadav-Barak

To verify, either way predictions and probabilities return as np array?

yeap

@noamzbr noamzbr merged commit 6b6c4cb into main May 11, 2023
21 of 22 checks passed
@delete-merged-branch delete-merged-branch bot deleted the yuriiromanyshyn/dee-456-unify-dummy-models branch May 11, 2023 06:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Making significant changes to structure of code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants