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

Callbacks #119

Merged
merged 91 commits into from Sep 4, 2019
Merged

Callbacks #119

merged 91 commits into from Sep 4, 2019

Conversation

zoq
Copy link
Member

@zoq zoq commented Jun 2, 2019

This is based on #113; see #49 for more details.

Hard to do with such crazy template metaprogramming...
Needs Armadillo sparse vectorise() support next.
@rcurtin rcurtin mentioned this pull request Jul 12, 2019
@rcurtin
Copy link
Member

rcurtin commented Aug 20, 2019

If the BBSGD tests are failing and don't seem to be a part of this PR, we can open a separate issue for that. No need to hold this up on those; let me know what you think. 👍

double finalValue = f.Evaluate(coords);
REQUIRE(finalValue <= 1e-5);
for (size_t j = 0; j < 4; ++j)
REQUIRE(coords[j] <= 1e-3);
Copy link
Contributor

Choose a reason for hiding this comment

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

I recommend using the element accessor with bounds check here and in other code like this, ie. coords(j) instead of coords[j].

Since this is a test case, we should catch all possible errors. Using [] allows some errors to slip through.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, good point. I did a little regex-ery and here's a patch we can apply to this branch that makes this change for all tests:

https://gist.github.com/e70df70cdb56072d28940ec197ba8036

Copy link
Member Author

Choose a reason for hiding this comment

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

Great, will apply the patch.

Copy link
Member

@rcurtin rcurtin left a comment

Choose a reason for hiding this comment

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

This is a huge PR but from my view I think everything is ready. Once this is merged, I'll go ahead and release ensmallen 2.10.0.

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

@zoq zoq merged commit 45efb3e into mlpack:master Sep 4, 2019
@rcurtin rcurtin mentioned this pull request Sep 7, 2019
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

4 participants