Skip to content

Add IsInf to detect infinity values#1884

Merged
linkerzhang merged 12 commits intoonnx:masterfrom
wschin:update-isnan
Apr 18, 2019
Merged

Add IsInf to detect infinity values#1884
linkerzhang merged 12 commits intoonnx:masterfrom
wschin:update-isnan

Conversation

@wschin
Copy link
Copy Markdown
Collaborator

@wschin wschin commented Mar 22, 2019

In addition to NaN, positive and negative values are also very important special values in floating-number system, so it'd be nice if IsInf can support them.

@prasanthpul prasanthpul requested review from houseroad and linkerzhang and removed request for houseroad March 28, 2019 15:59
Comment thread docs/Changelog.md Outdated
#### Attributes

<dl>
<dt><tt>detectnegativeinfinity</tt> : int (default is 0)</dt>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Use the same style for attribute naming as the rest of ONNX. The above should be 'detect_negative_infinity'.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Test files are added and now the strategy becomes

  • Not to touch IsNaN
  • Add IsInf instead.

Comment thread docs/Changelog.md Outdated

<dl>
<dt><tt>detectnegativeinfinity</tt> : int (default is 0)</dt>
<dd>(Optional) Whether map negative infinity to true. Default to 1 so that negative infinity induces true. Set this attribute to 0 if positive infinity should be mapped to false.</dd>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

"positive" => "negative"

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Ok.

Comment thread onnx/defs/tensor/defs.cc Outdated
"detectnegativeinfinity",
"(Optional) Whether map negative infinity to true. Default to 1 "
"so that negative infinity induces true. Set this attribute to 0 "
"if positive infinity should be mapped to false.",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

"positive" => "negative"

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Ok.

@gramalingam
Copy link
Copy Markdown
Contributor

Adding a test-case to test this would be helpful.

@spandantiwari
Copy link
Copy Markdown
Contributor

@wschin - This is useful functionality and will be a welcome addition to ONNX.

But have we considered an alternate design where we have a separate op for detecting Inf and -Inf, possibly named IsInf?

Inf and NaN are two different floating-point numbers with totally different meaning and IEEE representation. Putting Infs under the IsNan op is not the best design. I am aware that we do not want to add too many ops to the spec, but I think this is one of those cases where clarity of design trumps that concern with adding another op. As example, they are separate ops in TF, Numpy, MATLAB, and PyTorch (in PyTorch there is no isinf, but isnan is just for NaNs not Infs).

@wschin
Copy link
Copy Markdown
Collaborator Author

wschin commented Apr 3, 2019

@spandantiwari, sounds good. I will update this PR to add IsInf with detect_positive and detect_negative attributes.

@spandantiwari
Copy link
Copy Markdown
Contributor

@wschin - OK, that sounds good. Thanks for your consideration.

@wschin wschin changed the title Update IsNaN to detect infinity values Add IsInf to detect infinity values Apr 3, 2019
Copy link
Copy Markdown
Member

@houseroad houseroad 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.

@wschin wschin requested a review from a team as a code owner April 17, 2019 14:38
@spandantiwari
Copy link
Copy Markdown
Contributor

@spandantiwari, sounds good. I will update this PR to add IsInf with detect_positive and detect_negative attributes.

LGTM.

@linkerzhang linkerzhang merged commit ad03640 into onnx:master Apr 18, 2019
@wschin wschin deleted the update-isnan branch April 28, 2019 00:16
hariharans29 pushed a commit to hariharans29/onnx that referenced this pull request Aug 15, 2019
* Add IsInf

* Add test files

* Fix attribute names

* Format code

* Update doc

* Add coverage change
jcwchen pushed a commit to jcwchen/onnx that referenced this pull request Sep 23, 2020
* Add IsInf

* Add test files

* Fix attribute names

* Format code

* Update doc

* Add coverage change
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.

6 participants