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

Factour out some functions from llm-utils repository #25

Merged
merged 7 commits into from
Dec 22, 2023
Merged

Conversation

nicovank
Copy link
Collaborator

A first set of factoring out of common method re-using those already in llm-utils.

This removes duplicate code but will also improve accuracy / quality of output.

Test Plan:

% pip install .
% g++ -g ./test/test-failed-assert.cpp
% gdb ./a.out
(gdb) % run
(gdb) % why
The issue arises from the function `fact(float n)` defined in the file `test-
failed-assert.cpp`. Specifically, the issue lies in the for-loop where the
factorial is being calculated.

In this loop, `i` is initialized to `0.0` and then on each iteration, the
variable `x` is multiplied by `i`. Thus, in the first iteration, `x` is zero
because it's being multiplied by `i`, which is `0.0`. The loop therefore sets
`x` to zero and it remains zero in all subsequent iterations, regardless of the
value of `n`. Then, immediately after the loop, there's an assertion checking
that `x` is not zero. Given `x` is always zero due to the loop, this assertion
always fails and calls `abort()`, resulting in the SIGABRT signal recorded in
the stack trace.

To fix this issue, start the for-loop counter `i` from `1.0` instead of `0.0`.
This will ensure that `x` is not set to zero on the first iteration, and give a
correct calculation for the factorial of `n`. The modified function would look
like this:

```c++
float fact(float n) {
  auto x = 1.0;
  for (auto i = 1.0; i <= n; i++) {
    x *= i;
  }
  assert(x != 0.0);
  return x;
}
```

This adjustment will skip the 0 and start multiplication from 1, so the function
should now calculate the factorial correctly for values of `n` greater than 0,
without failing the assertion.
(Total cost: approximately $0.04 USD.)

Copy link
Contributor

@khlevin khlevin left a comment

Choose a reason for hiding this comment

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

Looks good to me! :)

@khlevin khlevin merged commit 53e72c3 into main Dec 22, 2023
2 checks passed
@khlevin khlevin deleted the llm-utils branch December 22, 2023 02:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants