[PATCH] fix 'positiveRemainder' in SINT

[Qian Yun]

(1) -> positiveRemainder(-7, -3)$SINT

(1) - 4
Type: SingleInteger


positiveRemainder : (%, %) -> %
++ positiveRemainder(a, b) (where \spad{b > 1}) yields r
++ where \spad{0 <= r < b} and \spad{r = a rem b}.

I think the documentation for positiveRemainder needs a change
as well.

- Qian

(Disclaimer: this issue is found by LLM, but the analysis and patch
is thoroughly reviewed by me.)

diff --git a/src/algebra/si.spad b/src/algebra/si.spad
index 85abf0d3..c836d8b9 100644
--- a/src/algebra/si.spad
+++ b/src/algebra/si.spad
@@ -282,7 +282,7 @@
positiveRemainder(x, n) ==
r : % := rem_SI(x, n)$Lisp
negative?_SI(r)$Lisp =>
- negative?_SI(n)$Lisp => sub_SI(x, n)$Lisp
+ negative?_SI(n)$Lisp => sub_SI(r, n)$Lisp
add_SI(r, n)$Lisp
r

[Ralf Hemmecke]

HI Qian,

the docstring clearly says b>1. You cannot expect that the rest of the
specification is fulfilled, if the input condition is violated.

Actually it seems that the code is doing more than it needs to do.

Ralf

[Waldek Hebisch]

On Mon, Apr 20, 2026 at 06:56:14AM +0800, Qian Yun wrote:
> (1) -> positiveRemainder(-7, -3)$SINT
>
> (1) - 4
> Type: SingleInteger
>
>
> positiveRemainder : (%, %) -> %
> ++ positiveRemainder(a, b) (where \spad{b > 1}) yields r
> ++ where \spad{0 <= r < b} and \spad{r = a rem b}.
>
> I think the documentation for positiveRemainder needs a change
> as well.
>
> - Qian

As Ralf wrote documentation only promises that positiveRemainder
works for b > 1. So one possible fix would be to remove code for
case when b < -1. Since orignal author clearly wanted to support
such case it is probably better to fix it. I am tempted to keep
this as an undocumented extention, that is keep condition 'b > 1'
in the documentation. The 'r = a rem b' is rather unclear,
I would rather write 'a = q*b + r'.

> (Disclaimer: this issue is found by LLM, but the analysis and patch
> is thoroughly reviewed by me.)
>
> diff --git a/src/algebra/si.spad b/src/algebra/si.spad
> index 85abf0d3..c836d8b9 100644
> --- a/src/algebra/si.spad
> +++ b/src/algebra/si.spad
> @@ -282,7 +282,7 @@
> positiveRemainder(x, n) ==
> r : % := rem_SI(x, n)$Lisp
> negative?_SI(r)$Lisp =>
> - negative?_SI(n)$Lisp => sub_SI(x, n)$Lisp
> + negative?_SI(n)$Lisp => sub_SI(r, n)$Lisp
> add_SI(r, n)$Lisp
> r
>

> --
> You received this message because you are subscribed to the Google Groups "FriCAS - computer algebra system" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to fricas-devel...@googlegroups.com.
> To view this discussion visit https://groups.google.com/d/msgid/fricas-devel/3a40918b-088a-4ed0-aedf-271085df03c9%40gmail.com.

--
Waldek Hebisch

[Ralf Hemmecke]

On 4/20/26 20:09, Waldek Hebisch wrote:
>> positiveRemainder : (%, %) -> %
>> ++ positiveRemainder(a, b) (where \spad{b > 1}) yields r
>> ++ where \spad{0 <= r < b} and \spad{r = a rem b}.

> I am tempted to keep
> this as an undocumented extention, that is keep condition 'b > 1'
> in the documentation. The 'r = a rem b' is rather unclear,
> I would rather write 'a = q*b + r'.

Well, I would agree with this 'a = q*b + r' change, but we also have

https://github.com/fricas/fricas/blob/r1.3.13/src/algebra/catdef.spad#L436

divide : (%, %) -> Record(quotient : %, remainder : %)
++ divide(x, y) divides x by y producing a record containing a
++ \spad{quotient} and \spad{remainder},
++ where the remainder is smaller (see
\spadfunFrom{sizeLess?}{EuclideanDomain})
++ than the divisor y.
"quo" : (%,%) -> %
++ x quo y is the same as \spad{divide(x, y).quotient}.
++ See \spadfunFrom{divide}{EuclideanDomain}.
"rem": (%,%) -> %
++ x rem y is the same as \spad{divide(x, y).remainder}.
++ See \spadfunFrom{divide}{EuclideanDomain}.

Unfortunately, even the docstring of divide is similarly vague.
Maybe, we should add 'a = q*b + r' also to the docstring for "divide".

Ralf