Sebastian Švec svecs
svecs commented on pull request svecs/gleamath#1 2025-10-03 15:47:04 +00:00
main

replacing the inner case statement with something like

svecs commented on pull request svecs/gleamath#1 2025-10-03 15:47:04 +00:00
main

you can make sign an Int or a fr.Fr and refactor the function to make it much simpler (remove the inner case statement)

svecs commented on pull request svecs/gleamath#1 2025-10-03 15:47:04 +00:00
main

this huge comment block is not really necessary, just say that its taylor series and people can look that up if they want details

svecs commented on pull request svecs/gleamath#1 2025-10-03 15:47:04 +00:00
main

why are there two spaces now? the comment made by me was an example for you to follow and use one space like i always do

svecs commented on pull request svecs/gleamath#1 2025-10-03 15:46:50 +00:00
main

still not added

svecs commented on pull request svecs/gleamath#1 2025-10-03 15:45:40 +00:00
main

replacing the inner case statement with something like

// if sign is a fr.Fr
sin_sum(
  x,
  k - 1,
  add(sum, mul(sign, div(int_pow(x, 2 * k + 1), int(fact(2 * k + 1))))),
 
svecs commented on pull request svecs/gleamath#1 2025-10-03 15:33:18 +00:00
main

?

svecs suggested changes for svecs/gleamath#1 2025-10-02 20:58:10 +00:00
main

a few improvements could be made

svecs commented on pull request svecs/gleamath#1 2025-10-02 20:58:10 +00:00
main

this expression deserves a comment with the math formula (its not very readable due to the use of functions instead of math operators)

svecs commented on pull request svecs/gleamath#1 2025-10-02 20:58:10 +00:00
main

why is there a new module and this function is not in the fr module?

svecs commented on pull request svecs/gleamath#1 2025-10-02 20:58:10 +00:00
main

i would call the second argument digit_diff, the two extra characters wont kill you and its much more readable (dig_diff actually confused me for like a minute)

svecs commented on pull request svecs/gleamath#1 2025-10-02 20:58:10 +00:00
main

idk if this function is useful enough to be in the public api (probably not)

svecs commented on pull request svecs/gleamath#1 2025-10-02 20:58:10 +00:00
main

this function should just be called int_pow, not fr_int_pow, since its inside of the fr module already

svecs commented on pull request svecs/gleamath#1 2025-10-02 20:58:10 +00:00
main

preferably only use global constants when necessary. this constant is only used in one place (next function), you can just define it inside of the function and not pollute the global namespace

svecs commented on pull request svecs/gleamath#1 2025-10-02 20:58:10 +00:00
main

theres some uncanny valley vibes about the ordering of the lines and this line being here rather than after the current line 69 (nice)

svecs commented on pull request svecs/gleamath#1 2025-10-02 20:58:10 +00:00
main

you can actually merge the first two cases into one by using <= 0 instead of just < 0

svecs commented on pull request svecs/gleamath#1 2025-10-02 19:38:12 +00:00
main

nit: add a space between the slashes and the comment for readability

svecs pushed to main at svecs/gleamath 2025-09-16 06:15:26 +00:00
575271f915 division
svecs pushed to main at svecs/gleamath 2025-09-13 20:33:43 +00:00
6d3e4f646d constants
svecs pushed to main at svecs/gleamath 2025-09-13 20:13:41 +00:00
e98ef8306b minmax