-
Notifications
You must be signed in to change notification settings - Fork 4
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
Update CLI functions to use new classes #55
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good ! two minor comments:
- csv has been renamed to data_handlers
- in your branch, make docs is failing (outputing 2 warnings), run
make docs
locally to check. Probably a good idea to also run make check-quality and make check-types.
src/stimulus/cli/split_csv.py
Outdated
|
||
from stimulus.data.data_handlers import CsvProcessing | ||
from stimulus.utils.launch_utils import get_experiment | ||
from stimulus.data.csv import DatasetProcessor, SplitManager |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
csv has been renamed to data_handers to avoid shadowing csv python librairy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Closes #48
Closes #49
shuffle_csv and split_csv CLI functions have been updated to use the new classes. They should be functional now. There is an issue with the split_csv test (see #51), so the test is temporarily disabled.