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 callback return values and clarify docs #383

Merged
merged 6 commits into from
Nov 15, 2023

Conversation

rcurtin
Copy link
Member

@rcurtin rcurtin commented Nov 10, 2023

This is a sibling PR to #382 and part of fixing #377.

I went through and audited the code to find places where the return values of callback functions were being ignored. I modified optimizers as necessary to handle those return values, attempting to terminate optimization before any more steps were taken.

I also modified the documentation to clarify that bools should be returned. However, for BeginOptimization() and EndOptimization() returning a bool doesn't make sense---so I made those return void.

In a couple places there were various other cleanups that I did, but nothing that affected functionality in any real way unrelated to callbacks.

Once this is done/merged, I also plan to update the technical report's section on callbacks and re-upload to arXiv.

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

@conradsnicta conradsnicta merged commit f73995b into mlpack:master Nov 15, 2023
1 of 4 checks passed
@rcurtin rcurtin deleted the callbacks-bool branch November 15, 2023 14:15
@rcurtin
Copy link
Member Author

rcurtin commented Nov 15, 2023

I submitted an update to the arXiv technical report for ensmallen that clarifies the new callback structure.

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

3 participants