-
Notifications
You must be signed in to change notification settings - Fork 221
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
Add sigmoid(), sigmoid_approx(), sigmoid_affine() and sigmoid_affine_approx() functions #148
base: master
Are you sure you want to change the base?
Conversation
d5f4f90
to
7f21c7f
Compare
a8b0e71
to
bed9710
Compare
Ran a benchmark. Results on AMD Ryzen 9 5950X:
In theory, the fast one should be faster than the approximation, but that is not the case. Also the output range is different with -1 to 1 for the fast one. We should think about if we want to keep it. Test Code#include <cstdio>
#include <ctime>
#include <cstdlib>
#include <cmath>
#include <random>
#include <chrono>
constexpr int SIZE = 100;
constexpr int CYCLES = 10000000;
double benchmark(const char *name, double (*fun)(double)) {
double xs[SIZE];
std::mt19937_64 generator(std::chrono::steady_clock::now().time_since_epoch().count());
std::uniform_real_distribution distribution(-100.0, 100.0);
for (double & x : xs) {
x = distribution(generator);
}
clock_t start = clock();
for (int repeat = 0; repeat < CYCLES; repeat++) {
for (const double x : xs) {
(*fun)(x);
}
}
const clock_t stop = clock();
const double t_ns = (stop - start) * 1.0e9 / CLOCKS_PER_SEC / CYCLES / SIZE;
printf("%-17s %6.1f ns\n", name, t_ns);
return t_ns;
}
double sigmoid(double p_x) {
return 1.0 / (1.0 + ::exp(-p_x));
}
double sigmoid_approx(double p_x) { return 0.5 + p_x / (4.0 + fabs(p_x)); }
double sigmoid_fast(double p_x) { return p_x / (1.0 + fabs(p_x)); }
int main(int argc, char **argv) {
benchmark("sigmoid", sigmoid);
benchmark("sigmoid_approx", sigmoid_approx);
benchmark("sigmoid_fast", sigmoid_fast);
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can confirm, this Sigmoid code adds a few Sigmoid Functions...
YES! |
Shouldn't all output be [1..-1]? |
Well it depends how we want it. The output of the real / normal sigmoid is between 0 and 1. Overall I would vote for removing the sigmoid_fast() impl as its not the fastest. This would also remove the anomaly. |
Ah, right. Well, as long as they're all consistent. Transforming them could be done by the user, though maybe versions that take a y-transform could be added. Would also help to have an argument for the amplitude so those bits are done in C++, rather than GDScript, as those would be common things people would want to do with it. |
If you mean the y_translate as in your original example, then yeah, can do add that tomorrow. Anything else? Btw I couldn't find anything about your amplitude... seams like this is no valid sigmoid. |
Amplitude as in the Y-scale between bounds, so instead of being [0..1], you could pass it a 2 to make it [0..2] or something. |
Removed sigmoid_fast() and added sigmoid_affine() and sigmoid_affine_approx() with these benchmark results:
Test Code#include <cstdio>
#include <ctime>
#include <cstdlib>
#include <cmath>
#include <random>
#include <chrono>
constexpr int SIZE = 100;
constexpr int CYCLES = 10000000;
double benchmark(const char *name, double (*fun)(double)) {
double xs[SIZE];
std::mt19937_64 generator(std::chrono::steady_clock::now().time_since_epoch().count());
std::uniform_real_distribution distribution(-100.0, 100.0);
for (double & x : xs) {
x = distribution(generator);
}
clock_t start = clock();
for (int repeat = 0; repeat < CYCLES; repeat++) {
for (const double x : xs) {
(*fun)(x);
}
}
const clock_t stop = clock();
const double t_ns = (stop - start) * 1.0e9 / CLOCKS_PER_SEC / CYCLES / SIZE;
printf("%-17s %6.1f ns\n", name, t_ns);
return t_ns;
}
double benchmark_affine(const char *name, double (*fun)(double, double, double)) {
double xs[SIZE];
std::mt19937_64 generator(std::chrono::steady_clock::now().time_since_epoch().count());
std::uniform_real_distribution distribution(-100.0, 100.0);
for (double & x : xs) {
x = distribution(generator);
}
clock_t start = clock();
for (int repeat = 0; repeat < CYCLES; repeat++) {
for (const double x : xs) {
(*fun)(x, x, x);
}
}
const clock_t stop = clock();
const double t_ns = (stop - start) * 1.0e9 / CLOCKS_PER_SEC / CYCLES / SIZE;
printf("%-17s %6.1f ns\n", name, t_ns);
return t_ns;
}
double sigmoid(double p_x) {
return 1.0 / (1.0 + ::exp(-p_x));
}
double sigmoid_approx(double p_x) { return 0.5 + p_x / (4.0 + fabs(p_x)); }
double affine_sigmoid(double p_x, double p_amplitude, double p_y_translation) {
return p_amplitude / (1.0 + ::exp(-p_x)) + p_y_translation;
}
double affine_sigmoid_approx(double p_x, double p_amplitude, double p_y_translation) {
return p_amplitude * (0.5 + p_x / (4.0 + fabs(p_x))) + p_y_translation;
}
int main(int argc, char **argv) {
benchmark("sigmoid", sigmoid);
benchmark("sigmoid_approx", sigmoid_approx);
benchmark_affine("affine_sigmoid", affine_sigmoid);
benchmark_affine("affine_sigmoid_approx", affine_sigmoid_approx);
} |
aa76044
to
e33c07c
Compare
e33c07c
to
73260be
Compare
Nice work! That said, I thought you'd just overload the functions, since it's C++. Either way works, though. |
If you mean overloads in GDScript, that does only seem work if the overload is for a "Variant" type. But other than that, I reduced the c++ math funcs to only keep the affine version and call them with the original sigmoid values |
No, I'm talking about the C++ itself, hence why I said "since it's C++". Also why get rid of the versions that take only one argument? You can have both kinds, just overloading the function names, so one regular |
Okay now I get what you mean. Well, the way it is right now has the same effect, just less methods and still inlined. Maybe the compiler can even optimize away the extra multiplication by 1.0 |
Well, here's hoping it makes it in to Redot's GDScript! |
This needs to be squashed, would also like to push this upstream to Godot first, but that depends on whether @8bitprodigy and @NickUfer would be okay with that. |
Doesn't GitHub support squash on merge? Or is that a GitLab thing? And wasn't one problem with upstream Godot that they do not merge anything the community wants and even if, that it takes forever to merge? 🤔 |
The way Github does squashing ruins the git history, it ruins any reference to the PR, we rely on Godot's PR Workflow still for consistency and because it is actually useful. And even in that case, we try to stay up to date with Godot's upstream and if you haven't even tried a proposal with Godot then you can't actually know that, we prefer to look at stalled, abandoned, or rejected PRs that the community wants, those that weren't sent to Godot we'd prefer they'd be tried on Godot first to reduce the chance of merge conflicts and to give good will from ourselves despite what they did, with the exception for anyone who doesn't want to see their PR or proposal submitted upstream at all. |
3367196
to
2a1a59e
Compare
Well okay, then I guess this one gets forgotten. I personally do not care if this is in original Godot or not, but this already breaks some trust with what this project actually wants to achieve, to merge things the community wants, and this was a community proposal here. |
We're not going to let them sit for months before we decide to merge them, we've discussed that on the core team. If we send stuff up to Godot, it is to minimize merge conflicts on our end mostly. Let's say they have a recent PR for a fix, but drag their feet. We cherry pick it and merge it, and then 3 months from now they decide to finally merge it too. Now we risk a merge conflict that we didn't need to have. That's basically the reason for this. It also shows good faith on our part, that we aren't being overly tribalistic. When things are handled by sending a PR to Godot, it won't be 3 months of waiting. If they don't show interest within perhaps a week (the exact time table has not been settled on), we will then pull the trigger on a merge into Redot |
We need to iron our a public facing statement for this, Since we are using the patch version (e.g 4.3.1) where Godot does not, it might be a good idea to tag it out for a patch release. That will give us a timeline to track, and when Godot abandons it we'll already have a plan in place to merge things in our backlog. My personal opinion: I do not think Godot is going to suddenly grow a sense of urgency because we exist, we can PR them all we want, and it will most likely be met with much the same response.. As for the merge conflict issue, i think we might be creating our own problem with that, by sending them up to Godot we are increasing the chance of a merge conflict, because since we know that Godot is slow to act. it means we will have to make a decision before they do in nearly all cases.. So that means had we just merged it in the first place we would have had to do less work. |
Agree with everything decrypted said, going to discuss with the core guys on reassessing the position. |
This PR still needs its documentation to be fixed. @NickUfer |
I'd be okay with it, though I only created the issue this is responding to. Nick did all the work. |
I am excited to hear this! |
2a1a59e
to
2d1153c
Compare
So the solution to this is not creating a pull request upstream in the first place so this problem can never happen? |
2d1153c
to
056398a
Compare
It's kinda ridiculous but yeah, that's pretty much where we're at. An upstream PR increases our risk of merge conflict. Appreciate the squash, will get this in front of the core team as we can review it |
Could you add tests in |
056398a
to
e2ca960
Compare
Tests were added, final fixes to descriptions (wrong function / variable names) are done. I think it is ready |
Could you rename the commit message to |
e2ca960
to
5202410
Compare
Done |
Could you capitalize the first character of the commit message? |
5202410
to
a88df50
Compare
I honestly do not understand what worth this has, but it is done. I can understand if the commit message needs "feat, fix, chore", etcat the beginning to trigger CI and select the next correct version, but upper case or lower case... waste of time? |
We don't use conventional commit standards because Godot never uses conventional commit standards, we want standardized commit messages in keeping with how Godot's commit messages are styled, which are styled minimally as I instructed you to do, (we generally try to follow Godot's PR workflow, see PR Workflow, PR Review Process, and CONTRIBUTING.md) it keeps the history clean, concise, and consistent (especially for changelogs) the CI doesn't run only because it wasn't approved by an admin to run. |
Closes Redot-Engine/redot-proposals#12