Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

typo in the operators docs of topk. #5826

Closed
wants to merge 5 commits into from
Closed

typo in the operators docs of topk. #5826

wants to merge 5 commits into from

Conversation

wplf
Copy link

@wplf wplf commented Dec 28, 2023

Description

Delete the redundant dimension in the operators doc of Topk.

before : shape [a_1, a_2, ..., a_n, r]
after: shape [a_1, a_2, ..., a_n]

Motivation and Context

@wplf wplf requested a review from a team as a code owner December 28, 2023 11:08
@wplf wplf changed the title typoype in topks of Operators typo in topk of Operators docs Dec 28, 2023
@wplf wplf changed the title typo in topk of Operators docs typo in the operators docs of topk. Dec 28, 2023
Copy link

codecov bot commented Dec 28, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (f1c59a5) 56.45% compared to head (5d93ab8) 56.45%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5826   +/-   ##
=======================================
  Coverage   56.45%   56.45%           
=======================================
  Files         504      504           
  Lines       29865    29865           
  Branches     4484     4484           
=======================================
  Hits        16860    16860           
  Misses      12188    12188           
  Partials      817      817           

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

@@ -33047,7 +33047,7 @@ expect(node, inputs=[x, repeats], outputs=[z], name="test_tile_precomputed")
### <a name="TopK"></a><a name="topk">**TopK**</a>

Retrieve the top-K largest or smallest elements along a specified axis. Given an input tensor of
shape [a_1, a_2, ..., a_n, r] and integer argument k, return two outputs:
shape [a_1, a_2, ..., a_n] and integer argument k, return two outputs:
Copy link
Contributor

Choose a reason for hiding this comment

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

The documentation needs to be be fixed as well in cc files. You may look for and integer argument k, return two outputs in the code to find all the location this string appears.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I find this typo appears in many files. Do I need to fix them all?
image

Copy link
Contributor

Choose a reason for hiding this comment

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

Those files are auto-generated. The sources need to be fixed: other than the defs.cc file already updated, we need to update the old.cc file in the same directory for older opset versions of the op.

Eg this line and this line

Copy link
Contributor

Choose a reason for hiding this comment

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

For this kind of simple change, it usually works if all instances of the same sentance are fixed. The auto-generation can be skipped. However, if you are curions, you need to build onnx and execute the following command lines:

pip install onnx -e . -v
# python onnx/backend/test/cmd_tools.py generate-data --clean  # needed if you add new backend test examples
# python onnx/backend/test/stat_coverage.py    # needed if you add new backend test examples
python onnx/defs/gen_doc.py  # generates the documentation

Copy link
Contributor

Choose a reason for hiding this comment

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

Just to add to Xavier's comments: the CI has a mechanism for automatically generate the documentation (using the auto update doc label), but it seems to be not working right now due to some git-issue that needs to be fixed.

@gramalingam gramalingam added the auto update doc Generate md/proto files automatically using the CI pipeline label Jan 3, 2024
liqunfu and others added 2 commits January 4, 2024 07:27
### Description
release build breaks due to onnx#5806
filesystem not available on Mac 10.12.
also need to skip 2 reference implement tests
(test_qlinearmatmul_3D_int8_float16_cpu,
test_qlinearmatmul_3D_int8_float32_cpu) which are failing on MacOS
(onnx#5792)

### Motivation and Context
fix mac release CI

---------

Signed-off-by: Liqun Fu <liqfu@microsoft.com>
Signed-off-by: Justin Chu <justinchuby@users.noreply.github.com>
Co-authored-by: Justin Chu <justinchuby@users.noreply.github.com>
Signed-off-by: wplf <975761915@qq.com>
Signed-off-by: wplf <975761915@qq.com>
@justinchuby justinchuby removed the auto update doc Generate md/proto files automatically using the CI pipeline label Jan 4, 2024
@gramalingam
Copy link
Contributor

PR #5948 redoes this

github-merge-queue bot pushed a commit that referenced this pull request Feb 27, 2024
### Description
Redo of PR #5826 (which has stalled), extended to address another issue
in the documentation.

Signed-off-by: Ganesan Ramalingam <grama@microsoft.com>
isdanni pushed a commit to isdanni/onnx that referenced this pull request Mar 18, 2024
### Description
Redo of PR onnx#5826 (which has stalled), extended to address another issue
in the documentation.

Signed-off-by: Ganesan Ramalingam <grama@microsoft.com>
Signed-off-by: isdanni <leedanni@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants