Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
rcurtin merged 4 commits into from
Nov 27, 2023
Merged

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

rcurtin 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.

@rcurtin rcurtin requested a review from NippunSharma November 24, 2023 17:25
This was referenced Nov 24, 2023
Closed
Closed
NippunSharma
NippunSharma approved these changes Nov 24, 2023
View reviewed changes
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

NippunSharma commented Nov 24, 2023

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?

mlpack-bot[bot]
mlpack-bot bot approved these changes Nov 25, 2023
View reviewed changes
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
@rcurtin rcurtin deleted the py-split-doc-fix branch November 27, 2023 23:58
This was referenced May 26, 2024
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mlpack-bot mlpack-bot[bot] mlpack-bot[bot] approved these changes

@NippunSharma NippunSharma NippunSharma approved these changes

Assignees

No one assigned

Labels

c: automatic bindings c: documentation t: bugfix

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@rcurtin @NippunSharma