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 implementation of fonesca fleming problem function f1 and f2 type usage and negative signs. #223

Merged
merged 2 commits into from Aug 30, 2020

Conversation

@coatless
Copy link
Contributor

@coatless coatless commented Aug 27, 2020

Fixes the type issues that came up during a CRAN check submission on Solaris.

In file included from ../inst/include/ensmallen_bits/problems/problems.hpp:20:0,
from ../inst/include/ensmallen.hpp:77,
from ../inst/include/RcppEnsmallen.h:25,
from RcppExports.cpp:4:
../inst/include/ensmallen_bits/problems/fonseca_fleming_function.hpp: In member function ‘typename MatType::elem_type ens::test::FonsecaFlemingFunction::ObjectiveA::Evaluate(const MatType&)’:
../inst/include/ensmallen_bits/problems/fonseca_fleming_function.hpp:74:57: error: call of overloaded ‘sqrt(int)’ is ambiguous
return 1.0f - exp(-pow(coords[0] - 1.0f / sqrt(3), 2) -

https://www.r-project.org/nosvn/R.check/r-patched-solaris-x86/RcppEnsmallen-00install.html

Also, removes an extra negative sign between x1 and x2 addition.

coatless added 2 commits Aug 27, 2020
- Improves the mathematical description of the problem
- Fixes the implementation of f1 and f2
- Ensure use of `pow()` and `sqrt()` return double
- pow(coords[1] - 1.0f / sqrt(3), 2)
- pow(coords[2] - 1.0f / sqrt(3), 2));
return 1.0 - exp(
-pow(static_cast<double>(coords[0]) - 1.0 / sqrt(3.0), 2.0)

This comment has been minimized.

@zoq

zoq Aug 27, 2020
Member

Looking at the error message: ‘sqrt(int)’ is ambiguous I think the static_cast is not necessary?

This comment has been minimized.

@coatless

coatless Aug 27, 2020
Author Contributor

It's probably overkill, I can remove if need be.

This comment has been minimized.

@rcurtin

rcurtin Aug 27, 2020
Member

We don't actually know what coords[0] is, so I might suggest using MatType::elem_type maybe like this:

typedef MatType::elem_type T;
return T(1) - exp(-pow(coords[0] - T(1) / sqrt(T(3)), T(2))
    - pow(coords[1] - T(1) / sqrt(T(3)), T(2))
    - pow(coords[2] - T(1) / sqrt(T(3)), T(2)));

Or, something like that could work. Basically the idea is to force every internal type inside the expression to be the same as MatType::elem_type.

This comment has been minimized.

@coatless

coatless Aug 27, 2020
Author Contributor

@rcurtin I agree with a type cast. That said, we input into pow() and sqrt() may be either float, double, or long double per their function definitions.

So, MatType::elem_type includes unsigned int and int.

short long
mat = Mat<double>
dmat = Mat<double>
fmat = Mat<float>
cx_mat = Mat<cx_double>
cx_dmat = Mat<cx_double>
cx_fmat = Mat<cx_float>
umat = Mat<uword>
imat = Mat<sword>

This comment has been minimized.

@rcurtin

rcurtin Aug 27, 2020
Member

Ah, good point! In this case, I'd be fine forcing to double like you suggested.

@zoq
zoq approved these changes Aug 28, 2020
Copy link
Member

@zoq zoq left a comment

Ready from my side, thanks.

@mlpack-bot
mlpack-bot bot approved these changes Aug 29, 2020
Copy link

@mlpack-bot mlpack-bot bot left a comment

Second approval provided automatically after 24 hours. 👍

@mlpack-bot mlpack-bot bot removed the s: needs review label Aug 29, 2020
Copy link
Member

@rcurtin rcurtin left a comment

Thanks! 👍

@coatless want to help test the automatic release system by updating your local repo to master after this merge, then using the script scripts/ensmallen-release.sh coatless 2 14 2? If not it's perfectly okay. 😄

@rcurtin rcurtin merged commit 5df4088 into mlpack:master Aug 30, 2020
4 checks passed
4 checks passed
Memory Checks Build finished.
Details
Static Code Analysis Checks Build finished.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@coatless coatless deleted the coatless:patch-1 branch Aug 30, 2020
@coatless
Copy link
Contributor Author

@coatless coatless commented Aug 30, 2020

@rcurtin after updating my fork, I'm getting:

(base) ➜  ensmallen git:(master) ./scripts/ensmallen-release.sh coatless 2 14 2
git diff returned a nonzero result!
@rcurtin
Copy link
Member

@rcurtin rcurtin commented Aug 30, 2020

Hmm, is it a pristine copy of master? If you had some local change, you could always do git stash or if you don't need to keep it, just git reset --hard origin/master. Then I think the script might work. 😄

@rcurtin
Copy link
Member

@rcurtin rcurtin commented Aug 30, 2020

Thank you for helping play with this script, by the way. The more people use it, the more robust it gets. :)

@rcurtin
Copy link
Member

@rcurtin rcurtin commented Aug 31, 2020

@coatless I just went ahead and ran the script. I think in a future release I'll document that git diff error a bit better. 👍

@coatless
Copy link
Contributor Author

@coatless coatless commented Sep 2, 2020

@rcurtin sorry for the delay, start of new semester. RE: git diff, it's because of the zsh shell or git version available on mac. For some reason, it is not reading one of the bquotes properly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.