Skip to content

Create the convenience methods i() and o() on Node #34

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

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

justinchuby
Copy link
Member

@justinchuby justinchuby commented May 30, 2025

  • Added i() and o() convenience methods to Node for accessing input and output values by index, respectively.

@justinchuby justinchuby requested review from titaiwangms and a team as code owners May 30, 2025 02:29
Copy link

codecov bot commented May 30, 2025

Codecov Report

Attention: Patch coverage is 84.61538% with 2 lines in your changes missing coverage. Please review.

Project coverage is 73.59%. Comparing base (60f8692) to head (cfd679b).

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
src/onnx_ir/_core.py 80.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #34      +/-   ##
==========================================
+ Coverage   73.57%   73.59%   +0.02%     
==========================================
  Files          37       37              
  Lines        4492     4496       +4     
  Branches      902      902              
==========================================
+ Hits         3305     3309       +4     
- Misses        858      859       +1     
+ Partials      329      328       -1     

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

Signed-off-by: Justin Chu <[email protected]>
Signed-off-by: Justin Chu <[email protected]>
Signed-off-by: Justin Chu <[email protected]>
Signed-off-by: Justin Chu <[email protected]>
@justinchuby justinchuby added this to the 0.1.1 milestone Jun 2, 2025
def i(self, index: int = 0) -> Value | None:
"""Get the input value at the given index.

This is a convenience method that is equivalent to `self.inputs[index]`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add the new syntax self.inputs[index] can be replaced by?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added docstring. Does that look good to you?

Signed-off-by: Justin Chu <[email protected]>
Copy link
Collaborator

@titaiwangms titaiwangms left a comment

Choose a reason for hiding this comment

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

Indexing on node.ouputs is more intuitive to me..


The following is equivalent::

node.outputs[0] == node.o(0) == node.o() # Default index is 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel it's duplicated.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok. I will release 0.1.1 without this for now

@justinchuby justinchuby removed this from the 0.1.1 milestone Jun 3, 2025
@justinchuby justinchuby marked this pull request as draft June 4, 2025 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants