Add sigmoid(), sigmoid_approx(), sigmoid_affine() and sigmoid_affine_approx() functions #148

NickUfer · 2024-10-04T15:22:07Z

NickUfer · 2024-10-04T17:49:39Z

Ran a benchmark. Results on AMD Ryzen 9 5950X:

sigmoid              5.1 ns
sigmoid_approx       1.4 ns
sigmoid_fast         1.8 ns

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);
}

JohnnyThunder2

Can confirm, this Sigmoid code adds a few Sigmoid Functions...

8bitprodigy · 2024-10-05T23:20:59Z

Implements Redot-Engine/redot-proposals#12

YES!

8bitprodigy · 2024-10-05T23:22:43Z

Also the output range is different with -1 to 1 for the fast one.

Shouldn't all output be [1..-1]?

NickUfer · 2024-10-05T23:27:41Z

Also the output range is different with -1 to 1 for the fast one.

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.

8bitprodigy · 2024-10-05T23:41:53Z

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.

NickUfer · 2024-10-05T23:45:16Z

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.

8bitprodigy · 2024-10-05T23:58:35Z

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.

NickUfer · 2024-10-06T17:24:53Z

Removed sigmoid_fast() and added sigmoid_affine() and sigmoid_affine_approx() with these benchmark results:

sigmoid               5.0 ns
sigmoid_approx        1.4 ns
sigmoid_affine        5.0 ns
sigmoid_affine_approx 1.4 ns

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);
}

8bitprodigy · 2024-10-07T00:17:38Z

Nice work! That said, I thought you'd just overload the functions, since it's C++.

Either way works, though.

NickUfer · 2024-10-07T12:30:47Z

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

8bitprodigy · 2024-10-07T22:14:28Z

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 sigmoid with its affine version also being called sigmoid.

NickUfer · 2024-10-07T22:23:42Z

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

8bitprodigy · 2024-10-09T08:27:12Z

Well, here's hoping it makes it in to Redot's GDScript!

Spartan322 · 2024-10-14T12:59:17Z

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.

NickUfer · 2024-10-14T15:05:06Z

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? 🤔

Spartan322 · 2024-10-14T21:35:54Z

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.

NickUfer · 2024-10-17T21:27:53Z

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.

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.

SkogiB · 2024-10-17T22:18:11Z

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.

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

SkogiB · 2024-10-17T22:29:29Z

Also want to apologize for the delays on this PR, we've just been busy. I do want this one merged (and cherrypicked to 4.3, the 4.3 pipeline will be much faster due to not caring if Godot wants it at all)

You don't HAVE to submit a proposal + PR to Godot, it just does the legwork for us on that front. We can cherry pick things up to them if we feel the need. If proposals were rejected by Godot, that expedites that process on our front, we know they don't want it and thus we don't foresee merge conflicts.

We ran the workflow and it failed the static check, I included a screenshot on that one. Pre-Commit is highly recommended to help catch these faster as well. If you get that fixed we'll look at it and see where we're at.

Are there any existing PRs/proposals for a sigmoid function in Godot? I feel like I saw that. Seeing they rejected it like a year ago or something would expedite our decision big time. Also, cherry pick it to 4.3 and make a PR and we can do stuff even faster for the stable builds.

decryptedchaos · 2024-10-17T22:30:50Z

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.

SkogiB · 2024-10-17T22:33:18Z

Agree with everything decrypted said, going to discuss with the core guys on reassessing the position.

Spartan322 · 2024-10-22T10:43:24Z

This PR still needs its documentation to be fixed. @NickUfer

8bitprodigy · 2024-10-24T03:22:49Z

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.

I'd be okay with it, though I only created the issue this is responding to. Nick did all the work.

8bitprodigy · 2024-10-24T03:24:25Z

I do want this one merged (and cherrypicked to 4.3, the 4.3 pipeline will be much faster due to not caring if Godot wants it at all)

I am excited to hear this!

SkogiB · 2024-10-24T06:46:24Z

Thanks for squashing, we just need this fixed and then squashed again and we should be good to go once it passes the workflow checks.

NickUfer · 2024-10-31T21:56:58Z

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.

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

So the solution to this is not creating a pull request upstream in the first place so this problem can never happen?

SkogiB · 2024-11-02T05:03:11Z

So the solution to this is not creating a pull request upstream in the first place so this problem can never happen?

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

Spartan322 · 2024-11-02T05:07:51Z

Could you add tests in tests/core/math/test_math_funcs.h?

NickUfer · 2024-11-03T18:39:29Z

Tests were added, final fixes to descriptions (wrong function / variable names) are done. I think it is ready

Spartan322 · 2024-11-03T19:06:00Z

Could you rename the commit message to Add sigmoid math functions?

NickUfer · 2024-11-03T20:14:02Z

Done

Spartan322 · 2024-11-04T01:38:48Z

Could you capitalize the first character of the commit message?

NickUfer · 2024-11-04T11:51:59Z

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?

Spartan322 · 2024-11-04T13:25:57Z

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.

NickUfer force-pushed the sigmoid_funcs branch from d5f4f90 to 7f21c7f Compare October 4, 2024 16:46

Aleksandar-Argead approved these changes Oct 4, 2024

View reviewed changes

NickUfer force-pushed the sigmoid_funcs branch from a8b0e71 to bed9710 Compare October 4, 2024 17:26

filipworksdev approved these changes Oct 4, 2024

View reviewed changes

NickUfer requested a review from filipworksdev October 4, 2024 20:20

filipworksdev approved these changes Oct 4, 2024

View reviewed changes

JohnnyThunder2 approved these changes Oct 5, 2024

View reviewed changes

Spartan322 mentioned this pull request Oct 14, 2024

Add a Sigmoid function to GDScript. Redot-Engine/redot-proposals#12

Open

NickUfer force-pushed the sigmoid_funcs branch from aa76044 to e33c07c Compare October 6, 2024 17:27

NickUfer changed the title ~~feat: adds sigmoid(), sigmoid_approx() and sigmoid_fast() functions~~ feat: adds sigmoid(), sigmoid_approx(), sigmoid_affine() and sigmoid_affine_approx() functions Oct 6, 2024

Redot-Engine deleted a comment Oct 6, 2024

NickUfer force-pushed the sigmoid_funcs branch from e33c07c to 73260be Compare October 6, 2024 18:26

Spartan322 changed the title ~~feat: adds sigmoid(), sigmoid_approx(), sigmoid_affine() and sigmoid_affine_approx() functions~~ Add sigmoid(), sigmoid_approx(), sigmoid_affine() and sigmoid_affine_approx() functions Oct 14, 2024

Spartan322 added enhancement topic:core labels Oct 14, 2024

NickUfer force-pushed the sigmoid_funcs branch from 3367196 to 2a1a59e Compare October 15, 2024 23:56

Spartan322 mentioned this pull request Oct 26, 2024

[4.3] Overhaul CanvasItemEditor snap lines drawing and input #807

Merged

NickUfer force-pushed the sigmoid_funcs branch from 2a1a59e to 2d1153c Compare October 31, 2024 21:56

NickUfer force-pushed the sigmoid_funcs branch from 2d1153c to 056398a Compare October 31, 2024 21:59

SkogiB approved these changes Nov 2, 2024

View reviewed changes

Spartan322 approved these changes Nov 2, 2024

View reviewed changes

NickUfer force-pushed the sigmoid_funcs branch from 056398a to e2ca960 Compare November 3, 2024 18:38

NickUfer force-pushed the sigmoid_funcs branch from e2ca960 to 5202410 Compare November 3, 2024 20:13

Adds sigmoid math functions

a88df50

NickUfer force-pushed the sigmoid_funcs branch from 5202410 to a88df50 Compare November 4, 2024 11:50

Add sigmoid(), sigmoid_approx(), sigmoid_affine() and sigmoid_affine_approx() functions #148

Are you sure you want to change the base?

Add sigmoid(), sigmoid_approx(), sigmoid_affine() and sigmoid_affine_approx() functions #148

Conversation

NickUfer commented Oct 4, 2024 • edited by Spartan322 Loading

NickUfer commented Oct 4, 2024

JohnnyThunder2 left a comment

Choose a reason for hiding this comment

8bitprodigy commented Oct 5, 2024

8bitprodigy commented Oct 5, 2024

NickUfer commented Oct 5, 2024 • edited Loading

8bitprodigy commented Oct 5, 2024

NickUfer commented Oct 5, 2024

8bitprodigy commented Oct 5, 2024

NickUfer commented Oct 6, 2024

8bitprodigy commented Oct 7, 2024

NickUfer commented Oct 7, 2024

8bitprodigy commented Oct 7, 2024

NickUfer commented Oct 7, 2024

8bitprodigy commented Oct 9, 2024 • edited Loading

Spartan322 commented Oct 14, 2024 • edited Loading

NickUfer commented Oct 14, 2024 • edited Loading

Spartan322 commented Oct 14, 2024 • edited Loading

NickUfer commented Oct 17, 2024 • edited Loading

SkogiB commented Oct 17, 2024

SkogiB commented Oct 17, 2024

decryptedchaos commented Oct 17, 2024 • edited Loading

SkogiB commented Oct 17, 2024 • edited Loading

Spartan322 commented Oct 22, 2024 • edited Loading

8bitprodigy commented Oct 24, 2024

8bitprodigy commented Oct 24, 2024

SkogiB commented Oct 24, 2024

NickUfer commented Oct 31, 2024 • edited Loading

SkogiB commented Nov 2, 2024

Spartan322 commented Nov 2, 2024 • edited Loading

NickUfer commented Nov 3, 2024

Spartan322 commented Nov 3, 2024

NickUfer commented Nov 3, 2024

Spartan322 commented Nov 4, 2024 • edited Loading

NickUfer commented Nov 4, 2024

Spartan322 commented Nov 4, 2024 • edited Loading

NickUfer commented Oct 4, 2024 •

edited by Spartan322

Loading

NickUfer commented Oct 5, 2024 •

edited

Loading

8bitprodigy commented Oct 9, 2024 •

edited

Loading

Spartan322 commented Oct 14, 2024 •

edited

Loading

NickUfer commented Oct 14, 2024 •

edited

Loading

Spartan322 commented Oct 14, 2024 •

edited

Loading

NickUfer commented Oct 17, 2024 •

edited

Loading

decryptedchaos commented Oct 17, 2024 •

edited

Loading

SkogiB commented Oct 17, 2024 •

edited

Loading

Spartan322 commented Oct 22, 2024 •

edited

Loading

NickUfer commented Oct 31, 2024 •

edited

Loading

Spartan322 commented Nov 2, 2024 •

edited

Loading

Spartan322 commented Nov 4, 2024 •

edited

Loading

Spartan322 commented Nov 4, 2024 •

edited

Loading