Skip to content

Clarify Mod operator#6973

Merged
xadupre merged 2 commits intoonnx:mainfrom
cbourjau:issue-5564
May 22, 2025
Merged

Clarify Mod operator#6973
xadupre merged 2 commits intoonnx:mainfrom
cbourjau:issue-5564

Conversation

@cbourjau
Copy link
Copy Markdown
Contributor

@cbourjau cbourjau commented May 17, 2025

Description

Improve description of the Mod operator. Closes #5564

Motivation and Context

The description of the Mod operator has been very vague. The proposed change aligns with the behavior of onnxruntime and the onnx reference implementation.

Test against onnxruntime and onnx reference implementation The following code snippet checks the existing behavior of onnxruntime and the onnx reference implementation. This is not a stable feature of spox, yet, thus the private imports.

Execute using pixi

pixi exec -s onnxruntime -s onnx -s spox -s numpy python mod.py

or in any Python environment with onnx, onnxruntime and spox installed.

import math

import spox
import spox._future
import spox.opset.ai.onnx.v21 as op
import numpy as np

BACKENDS = [spox._value_prop.ValuePropBackend.ONNXRUNTIME, spox._value_prop.ValuePropBackend.REFERENCE]

def check_mod_impl(x, y, expected, fmod):
    for backend in BACKENDS:
        spox._future.set_value_prop_backend(backend)
        a = op.const(x)
        b = op.const(y)
        res = op.mod(a, b, fmod=fmod)._value.value

        np.testing.assert_equal(expected, res)

cases = [
    (3.0, 2.0),
    (3.0, -2.0),
    (-3.0, 2.0),
    (-3.0, -2.0),
]
for (x, y) in cases:
    expected = x - math.trunc(x/y) * y
    check_mod_impl(x, y, expected, fmod=1)

special_cases = [
    (-0.0, -1.0, -0.0),
    (-0.0, 1.0, -0.0),
    (float("inf"), 2.0, float("nan")),
    (3.0, float("inf"), 3.0),
    (float("nan"), 2.0, float("nan")),
    (2.0, float("nan"), float("nan")),
]
for (x, y, expected) in special_cases:
    check_mod_impl(x, y, expected, fmod=1)


cases = [
    (5, 3),
    (5, -3),
    (-5, 3),
    (-5, -3),
]
for (x, y) in cases:
    expected = x % y
    check_mod_impl(x, y, expected, fmod=0)

@cbourjau cbourjau requested a review from a team as a code owner May 17, 2025 19:41
@github-project-automation github-project-automation Bot moved this to In progress in PR Tracker May 17, 2025
@cbourjau cbourjau mentioned this pull request May 17, 2025
Closed
justinchuby
justinchuby reviewed May 17, 2025
View reviewed changes
Comment thread onnx/defs/math/defs.cc
@justinchuby justinchuby requested a review from Copilot May 18, 2025 02:26
Copilot AI reviewed May 18, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refines and expands the documentation of the Mod operator to clearly describe its behavior under both integer and floating-point modes, aligning the text with ONNX Runtime and reference implementations.

  • Rewrote the C++ schema comment to specify fmod attribute semantics, valid types, and special cases.
  • Mirror those documentation updates in the high-level Operators.md and Changelog.md.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File Description
onnx/defs/math/defs.cc Enhanced Mod operator docstring with detailed fmod rules and special-case behaviors.
docs/Operators.md Updated the Mod operator section to match the new schema description.
docs/Changelog.md Reflected the same expanded documentation in the changelog entry for Mod-13.

Comment thread onnx/defs/math/defs.cc Outdated
Comment thread onnx/defs/math/defs.cc Outdated
Comment thread docs/Operators.md Outdated
Comment thread docs/Changelog.md Outdated
@justinchuby justinchuby requested a review from Copilot May 18, 2025 13:40
Copilot AI reviewed May 18, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refines the documentation of the Mod operator across code and markdown, clarifying type constraints, fmod behavior, and special cases to match ONNX reference implementations and ONNX Runtime.

  • Update C++ schema doc for Mod with detailed semantics for fmod=0 (integer %) and fmod=1 (C fmod) including special floating-point cases.
  • Revise user-facing docs in Operators.md and Changelog.md to mirror the enhanced description.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
onnx/defs/math/defs.cc Enhanced Mod operator schema doc with precise behavior
docs/Operators.md Updated Mod operator markdown doc for clarity
docs/Changelog.md Aligned changelog entry for Mod operator v13 docs

@cbourjau
Clarify Mod operator
f90da0f 
Signed-off-by: Christian Bourjau <christian.bourjau@quantco.com>
cbourjau
cbourjau commented May 18, 2025
View reviewed changes
Comment thread docs/Changelog.md
The result of this operator is the remainder of the division operation `x / y` where `x` and `y` are respective elements of `A` and `B`. The result is exactly the value `x - n * y`, where `n` is `x / y` with its fractional part truncated.
The returned value has the same sign as `x` (except if `x` is `-0`) and is less or equal to `|y|` in magnitude.
The following special cases apply when `fmod` is set to `1`:
- If `x` is `-0` and `y` is greater than zero, either `+0` or `-0` may be returned.
Copy link
Copy Markdown
Contributor Author

@cbourjau cbourjau May 18, 2025

Choose a reason for hiding this comment

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

I should point out that currently, both onnxruntime and the reference implementation return the sign of x even in that case. However, the array-api standard points out this special case, which makes me suspect that some backends would have an issue here.

@cbourjau
Maintain backwards compatible wording
29e1120 
Signed-off-by: Christian Bourjau <christian.bourjau@quantco.com>
@github-project-automation github-project-automation Bot moved this from In progress to Reviewer approved in PR Tracker May 19, 2025
@xadupre xadupre enabled auto-merge May 19, 2025 12:15
@justinchuby justinchuby requested a review from gramalingam May 19, 2025 13:58
@xadupre xadupre added this pull request to the merge queue May 22, 2025
@justinchuby justinchuby added this to the 1.19 milestone May 22, 2025
Merged via the queue into onnx:main with commit 281f630 May 22, 2025
37 checks passed
@github-project-automation github-project-automation Bot moved this from Reviewer approved to Done in PR Tracker May 22, 2025
@justinchuby justinchuby added module: spec topic: spec clarification Clarification of the ONNX spec needed labels May 22, 2025
KarelZe pushed a commit to KarelZe/onnx that referenced this pull request May 23, 2025
@cbourjau @KarelZe
Clarify Mod operator (onnx#6973)
4141725 
### Description
Improve description of the `Mod` operator. Closes onnx#5564

### Motivation and Context
The description of the `Mod` operator has been very vague. The proposed
change aligns with the behavior of onnxruntime and the onnx reference
implementation.

<details>
<summary>Test against onnxruntime and onnx reference
implementation</summary>
The following code snippet checks the existing behavior of onnxruntime
and the onnx reference implementation. This is not a stable feature of
spox, yet, thus the private imports.

Execute using pixi
```
pixi exec -s onnxruntime -s onnx -s spox -s numpy python mod.py
```

or in any Python environment with onnx, onnxruntime and spox installed.

```python
import math

import spox
import spox._future
import spox.opset.ai.onnx.v21 as op
import numpy as np

BACKENDS = [spox._value_prop.ValuePropBackend.ONNXRUNTIME, spox._value_prop.ValuePropBackend.REFERENCE]

def check_mod_impl(x, y, expected, fmod):
    for backend in BACKENDS:
        spox._future.set_value_prop_backend(backend)
        a = op.const(x)
        b = op.const(y)
        res = op.mod(a, b, fmod=fmod)._value.value

        np.testing.assert_equal(expected, res)

cases = [
    (3.0, 2.0),
    (3.0, -2.0),
    (-3.0, 2.0),
    (-3.0, -2.0),
]
for (x, y) in cases:
    expected = x - math.trunc(x/y) * y
    check_mod_impl(x, y, expected, fmod=1)

special_cases = [
    (-0.0, -1.0, -0.0),
    (-0.0, 1.0, -0.0),
    (float("inf"), 2.0, float("nan")),
    (3.0, float("inf"), 3.0),
    (float("nan"), 2.0, float("nan")),
    (2.0, float("nan"), float("nan")),
]
for (x, y, expected) in special_cases:
    check_mod_impl(x, y, expected, fmod=1)

cases = [
    (5, 3),
    (5, -3),
    (-5, 3),
    (-5, -3),
]
for (x, y) in cases:
    expected = x % y
    check_mod_impl(x, y, expected, fmod=0)
```

</details>

---------

Signed-off-by: Christian Bourjau <christian.bourjau@quantco.com>
KarelZe pushed a commit to KarelZe/onnx that referenced this pull request May 23, 2025
@cbourjau @KarelZe
Clarify Mod operator (onnx#6973)
4fd3f15 
### Description
Improve description of the `Mod` operator. Closes onnx#5564

### Motivation and Context
The description of the `Mod` operator has been very vague. The proposed
change aligns with the behavior of onnxruntime and the onnx reference
implementation.

<details>
<summary>Test against onnxruntime and onnx reference
implementation</summary>
The following code snippet checks the existing behavior of onnxruntime
and the onnx reference implementation. This is not a stable feature of
spox, yet, thus the private imports.

Execute using pixi
```
pixi exec -s onnxruntime -s onnx -s spox -s numpy python mod.py
```

or in any Python environment with onnx, onnxruntime and spox installed.

```python
import math

import spox
import spox._future
import spox.opset.ai.onnx.v21 as op
import numpy as np

BACKENDS = [spox._value_prop.ValuePropBackend.ONNXRUNTIME, spox._value_prop.ValuePropBackend.REFERENCE]

def check_mod_impl(x, y, expected, fmod):
    for backend in BACKENDS:
        spox._future.set_value_prop_backend(backend)
        a = op.const(x)
        b = op.const(y)
        res = op.mod(a, b, fmod=fmod)._value.value

        np.testing.assert_equal(expected, res)

cases = [
    (3.0, 2.0),
    (3.0, -2.0),
    (-3.0, 2.0),
    (-3.0, -2.0),
]
for (x, y) in cases:
    expected = x - math.trunc(x/y) * y
    check_mod_impl(x, y, expected, fmod=1)

special_cases = [
    (-0.0, -1.0, -0.0),
    (-0.0, 1.0, -0.0),
    (float("inf"), 2.0, float("nan")),
    (3.0, float("inf"), 3.0),
    (float("nan"), 2.0, float("nan")),
    (2.0, float("nan"), float("nan")),
]
for (x, y, expected) in special_cases:
    check_mod_impl(x, y, expected, fmod=1)

cases = [
    (5, 3),
    (5, -3),
    (-5, 3),
    (-5, -3),
]
for (x, y) in cases:
    expected = x % y
    check_mod_impl(x, y, expected, fmod=0)
```

</details>

---------

Signed-off-by: Christian Bourjau <christian.bourjau@quantco.com>
Signed-off-by: Markus Bilz <github@markusbilz.com>
geckosecurity pushed a commit to geckosecurity/onnx that referenced this pull request May 29, 2025
@cbourjau @nkoorty
Clarify Mod operator (onnx#6973)
fab27c4 
### Description
Improve description of the `Mod` operator. Closes onnx#5564

### Motivation and Context
The description of the `Mod` operator has been very vague. The proposed
change aligns with the behavior of onnxruntime and the onnx reference
implementation.

<details>
<summary>Test against onnxruntime and onnx reference
implementation</summary>
The following code snippet checks the existing behavior of onnxruntime
and the onnx reference implementation. This is not a stable feature of
spox, yet, thus the private imports.

Execute using pixi
```
pixi exec -s onnxruntime -s onnx -s spox -s numpy python mod.py 
```

or in any Python environment with onnx, onnxruntime and spox installed.

```python
import math

import spox
import spox._future
import spox.opset.ai.onnx.v21 as op
import numpy as np

BACKENDS = [spox._value_prop.ValuePropBackend.ONNXRUNTIME, spox._value_prop.ValuePropBackend.REFERENCE]

def check_mod_impl(x, y, expected, fmod):
    for backend in BACKENDS:
        spox._future.set_value_prop_backend(backend)
        a = op.const(x)
        b = op.const(y)
        res = op.mod(a, b, fmod=fmod)._value.value

        np.testing.assert_equal(expected, res)

cases = [
    (3.0, 2.0),
    (3.0, -2.0),
    (-3.0, 2.0),
    (-3.0, -2.0),
]
for (x, y) in cases:
    expected = x - math.trunc(x/y) * y
    check_mod_impl(x, y, expected, fmod=1)

special_cases = [
    (-0.0, -1.0, -0.0),
    (-0.0, 1.0, -0.0),
    (float("inf"), 2.0, float("nan")),
    (3.0, float("inf"), 3.0),
    (float("nan"), 2.0, float("nan")),
    (2.0, float("nan"), float("nan")),
]
for (x, y, expected) in special_cases:
    check_mod_impl(x, y, expected, fmod=1)


cases = [
    (5, 3),
    (5, -3),
    (-5, 3),
    (-5, -3),
]
for (x, y) in cases:
    expected = x % y
    check_mod_impl(x, y, expected, fmod=0)
```

</details>

---------

Signed-off-by: Christian Bourjau <christian.bourjau@quantco.com>
@osalbahr osalbahr mentioned this pull request Aug 27, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@justinchuby justinchuby justinchuby left review comments

Copilot code review Copilot Copilot left review comments

@gramalingam gramalingam gramalingam approved these changes

@cyyever cyyever cyyever approved these changes

@xadupre xadupre xadupre approved these changes

Assignees

No one assigned

Labels

module: spec topic: spec clarification Clarification of the ONNX spec needed

Projects

PR Tracker
Status: Done

Milestone

1.19

Development

Successfully merging this pull request may close these issues.

Mod operator docs could be clearer about which flavor of division is used by default

6 participants

@cbourjau @gramalingam @justinchuby @cyyever @xadupre