Skip to content

Conversation

@AyoubMDL
Copy link
Contributor

This PR adds the following transformation:

  • Min(Min(X)) -> Min(X)
  • Max(Max(X)) -> Max(X)
  • Min(Max(X)) -> Clip(X)
  • Max(Min(X)) -> Clip(X)

Copy link
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 implements fusion rules for successive Min/Max patterns in the ONNX rewriter. The PR adds optimizations to simplify chains of Min/Max operations by collapsing redundant operations and converting specific patterns to more efficient Clip operations.

  • Adds fusion of successive Min operations: Min(Min(X)) -> Min(X)
  • Adds fusion of successive Max operations: Max(Max(X)) -> Max(X)
  • Adds conversion of Min(Max(X)) and Max(Min(X)) patterns to Clip operations

Reviewed Changes

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

File Description
onnxscript/rewriter/min_max_to_clip.py Implements the core rewrite rules with abstract base class and concrete fusion implementations
onnxscript/rewriter/min_max_to_clip_test.py Comprehensive test suite covering successful fusion cases and failure conditions

@codecov
Copy link

codecov bot commented Aug 19, 2025

Codecov Report

❌ Patch coverage is 98.85057% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.17%. Comparing base (1934901) to head (e2125dc).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
...ipt/rewriter/rules/common/_min_max_to_clip_test.py 98.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2500      +/-   ##
==========================================
+ Coverage   69.99%   70.17%   +0.18%     
==========================================
  Files         216      218       +2     
  Lines       26074    26248     +174     
  Branches     2618     2625       +7     
==========================================
+ Hits        18250    18420     +170     
- Misses       6921     6923       +2     
- Partials      903      905       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@justinchuby
Copy link
Collaborator

Thanks! I think we can enable these patterns by default. Could you share on what models these patterns will appear?

@AyoubMDL
Copy link
Contributor Author

Thanks! I think we can enable these patterns by default. Could you share on what models these patterns will appear?

I originally added the rewrite for the Max(Min) → Clip fusion to handle cases where I needed to export a Keras model with a bounded ReLU to ONNX. For example, the following model uses ReLU(max_value=4.0):

import keras
import tensorflow as tf
import tf2onnx


def build_model(input_shape=(28, 28, 1), max_relu=4.0):
    inputs = keras.Input(shape=input_shape)
    x = keras.layers.Conv2D(8, (3, 3), padding="same")(inputs)
    outputs = keras.layers.ReLU(max_value=max_relu)(x)
    model = keras.Model(inputs=inputs, outputs=outputs)
    return model


model = build_model()

spec = (tf.TensorSpec((None, 28, 28, 1), tf.float32, name="input"),)

model_proto, _ = tf2onnx.convert.from_keras(
    model,
    input_signature=spec,
    opset=15,
    output_path="model.onnx",
)

This will generate the following onnx model which has Max(Min):
image

I added the adder rewrites mainly for generality, but I’m open to remove them if we feel they don’t add value.

@AyoubMDL AyoubMDL force-pushed the rewriter_min_max_fusion branch from c21bb05 to 28105cc Compare August 20, 2025 22:14
@justinchuby
Copy link
Collaborator

I think the rules are reasonable. I will look at them more closely

Copy link
Collaborator

@gramalingam gramalingam left a comment

Choose a reason for hiding this comment

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

LGTM ... had just a couple of minor comments/suggestions. Thanks!

@AyoubMDL AyoubMDL force-pushed the rewriter_min_max_fusion branch from 28105cc to 717fa45 Compare September 4, 2025 19:05
first_node: ir.Node,
second_node: ir.Node,
input_name: str = "",
) -> list[tuple[ir.Tensor, str]]: ...

Check notice

Code scanning / CodeQL

Statement has no effect Note

This statement has no effect.
@AyoubMDL AyoubMDL force-pushed the rewriter_min_max_fusion branch from 717fa45 to e2125dc Compare September 5, 2025 18:20
@justinchuby justinchuby enabled auto-merge (squash) September 5, 2025 19:12
@justinchuby justinchuby merged commit 5762a69 into microsoft:main Sep 5, 2025
55 of 56 checks passed
@github-project-automation github-project-automation bot moved this from Todo to Done in ONNX Script Review Board Sep 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Development

Successfully merging this pull request may close these issues.

4 participants