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

Fix call to preprocess_split() in generated documentation. #3563

Merged
merged 4 commits into from Nov 27, 2023

Conversation

rcurtin
Copy link
Member

@rcurtin rcurtin commented Nov 24, 2023

This is a fix for #3556, in the Python bindings. The generated documentation would print a line like

X_test, y_test, X_train, y_train = preprocess_split(input_=X, input_labels=y)

but preprocess_split() returns a Python dict with several members. Instead, this is what's necessary to get the training/test datasets:

d = preprocessd_split(input_=X, input_labels=y)
X_train = d['training']
y_train = d['training_labels']
X_test = d['test']
y_test = d['test_labels']

These changes update the function that prints this documentation to use the second version. I tested that the generated code runs successfully.

Copy link
Contributor

@NippunSharma NippunSharma left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@NippunSharma
Copy link
Contributor

As a side note: should we also look into changing what preprocess_split returns and change that to individual matrices. That seems to be a bit more "pythonic" way of doing it?

Copy link

@mlpack-bot mlpack-bot bot left a comment

Choose a reason for hiding this comment

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

Second approval provided automatically after 24 hours. 👍

@rcurtin
Copy link
Member Author

rcurtin commented Nov 27, 2023

@NippunSharma agreed, it would be great if we could make the preprocess_split() API cleaner, but I am not sure of a nice automated way to do that with our binding generation system. If you have ideas I am all ears 😄

@rcurtin rcurtin merged commit bbde929 into mlpack:master Nov 27, 2023
19 checks passed
@rcurtin rcurtin deleted the py-split-doc-fix branch November 27, 2023 23:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants