Skip to content

Fix Greater/LessOrEqual function definition#2641

Closed
take-cheeze wants to merge 1 commit intoonnx:masterfrom
take-cheeze:fix_great_less_eq_func
Closed

Fix Greater/LessOrEqual function definition#2641
take-cheeze wants to merge 1 commit intoonnx:masterfrom
take-cheeze:fix_great_less_eq_func

Conversation

@take-cheeze
Copy link
Copy Markdown
Member

@take-cheeze take-cheeze commented Mar 4, 2020

No description provided.

@take-cheeze take-cheeze requested a review from a team as a code owner March 4, 2020 10:41
@postrational
Copy link
Copy Markdown
Contributor

postrational commented Mar 4, 2020

Thanks for catching this. Please sign the CLA, if you haven't yet.

@take-cheeze
Copy link
Copy Markdown
Member Author

take-cheeze commented Mar 5, 2020

I don't know why CLA check isn't working...
Other PR is OK: #2640

@linkerzhang
Copy link
Copy Markdown
Member

linkerzhang commented Mar 6, 2020

Indeed a good catch!

One more question later is how to avoid this kind of bugs - validate function body's correctness. @jeremycochoy

@take-cheeze can you create a new PR to work around the CLA issue please? this PR has to be merged before the 1.7 release - which is happening now.

@linkerzhang
Copy link
Copy Markdown
Member

linkerzhang commented Mar 6, 2020

@chinhuang007

@jeremycochoy
Copy link
Copy Markdown
Contributor

jeremycochoy commented Mar 6, 2020

Indeed a good catch!

One more question later is how to avoid this kind of bugs - validate function body's correctness. @jeremycochoy

@take-cheeze can you create a new PR to work around the CLA issue please? this PR has to be merged before the 1.7 release - which is happening now.

Ouch. 🤦‍♂

Ideally, you'd like to "run" the function using an existing runtime and check against the function tests to confirm consistency between definition and implementation. But it my require a lots of work to setup a CI running a ONNX backend and executing tests...

@take-cheeze take-cheeze mentioned this pull request Mar 6, 2020
Merged
@take-cheeze take-cheeze deleted the fix_great_less_eq_func branch March 10, 2020 04:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@postrational postrational postrational approved these changes

+1 more reviewer

@linkerzhang linkerzhang linkerzhang approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@take-cheeze @postrational @linkerzhang @jeremycochoy