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 compilation warnings #115

Merged
merged 4 commits into from May 21, 2019

Conversation

@coatless
Copy link
Contributor

commented May 20, 2019

Addresses the variable initialization issue highlighted under -Wreorder, c.f.

 ../inst/include/ensmallen_bits/qhadam/qhadam_update.hpp:57:5: 
warning: field 'v2' will be initialized after field 'iteration' [-Wreorder]

@rcurtin + @zoq I can tackle #114 if need be in this as well.

Fix parameter initialization issue in qhadam_update
 Addresses the variable initialization issue highlighted under -Wreorder, c.f.

 ../inst/include/ensmallen_bits/qhadam/qhadam_update.hpp:57:5: warning: field 'v2' will be initialized after field 'iteration' [-Wreorder]
@zoq

This comment has been minimized.

Copy link
Member

commented May 20, 2019

Thanks for the report; have to set up the style checks, I think I said that before. Anyway, if you like let's include a fix for #114 here as well. Just checked and I think it's save to remove the shuffle parameter.

Remove unused `shuffle` parameter from `spsa`
Deletes all instances of `shuffle` from the `spsa` implementation

 ../inst/include/ensmallen_bits/spsa/spsa.hpp:120:8:
warning: private field 'shuffle' is not used [-Wunused-private-field]
  bool shuffle;
@coatless

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2019

@zoq added into this PR the removal of shuffle parameter in spsa highlighted by #114.

In another PR, I'll likely add the flags to the CMAKE Build here:

https://github.com/mlpack/ensmallen/blob/master/CMakeLists.txt#L36-L42

From the R docs, the flags I'll add are: -Wall -pedantic

https://cran.r-project.org/doc/manuals/r-release/R-exts.html#Writing-portable-packages

@rcurtin
Copy link
Member

left a comment

Thanks! I appreciate that you included the history update too. :)

@rcurtin

This comment has been minimized.

Copy link
Member

commented May 21, 2019

I realized we have to update doc/optimizers.md to remove the shuffle parameter for SPSA but that's easy and we can do it during merge. (Or if you want @coatless you can do it now, up to you.)

@coatless

This comment has been minimized.

Copy link
Contributor Author

commented May 21, 2019

@rcurtin removed the shuffle parameter from the function usage documentation.

Note: It wasn't included in the table of parameters.

@rcurtin

This comment has been minimized.

Copy link
Member

commented May 21, 2019

Note: It wasn't included in the table of parameters.

Yeah, only in the constructor, surprising we missed that during review. Thanks for fixing it. 👍

@zoq

zoq approved these changes May 21, 2019

Copy link
Member

left a comment

This looks good to me as well.

@zoq zoq merged commit 2be1d7b into mlpack:master May 21, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@zoq

This comment has been minimized.

Copy link
Member

commented May 21, 2019

Let's see if that fixes the style issues in #116.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.