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

implemented .inputs and .outputs properties for NIRGraphs #30

Merged
merged 4 commits into from
Oct 2, 2023

Conversation

matjobst
Copy link
Collaborator

Implementation of Issue #29 "Add .inputs and .outputs properties to NIRGraph"

Copy link
Collaborator

@Jegp Jegp left a comment

Choose a reason for hiding this comment

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

Really nice work @matjobst. I particularly like how we're not adding any data to the existing declarations, but just using the existing data to provide convenient functionality.

There are a few linter errors, but that may just be a matter of rebasing to main?

But, before we merge, I suggest we resolve the conversation here: #28

nir/ir.py Show resolved Hide resolved
nir/ir.py Outdated

@property
def inputs(self):
return {name:node for name,node in self.nodes.items() if isinstance(node, Input)}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I get a warning from ruff that the line is too long. May I suggest either adding pre-commit as mentioned in the docs or maybe bumping the minimum line length to something more than 88?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Or maybe it's just a matter of rebasing onto main?

@matjobst matjobst force-pushed the feature_inp_outp_properties branch from 083fc4b to 3e8e36d Compare July 25, 2023 07:51
@matjobst
Copy link
Collaborator Author

matjobst commented Jul 25, 2023

I pushed this failing version now to showcase one issue that came up:
In the tests (test_ir.py), line 155 I check the following:
assert ir.nodes["in2"] in ir2.nodes["inner"].inputs.values()
However, that fails with "ValueError: The truth value of an array with more than one element is ambiguous. Use a.any() or a.all()"

The reason for that is that dataclasses generate a default comparison __eq__() function which compares all attributes of the class, but does not understand how to properly compare the arrays. For the case of the Input node, the resulting __eq__() does something like this:

def __eq__(self, other):
    return self.shape == other.shape

Since the .shape is typically an array, it would rather need to do something like:

def __eq__(self, other):
    return (self.shape == other.shape).all()

However I am still not sure whether we want that behavior. Because that means that:

a = nir.Input(shape=np.array([1]))
b = nir.Input(shape=np.array([1]))
a == b   # True

This could be avoided by having a name in each node.
See also Issue #36 .

@Jegp
Copy link
Collaborator

Jegp commented Sep 4, 2023

#50 stabilizes how input and output types are treated, so now would be a good time to merge this, I believe. @matjobst, would you have time to fast-forward this branch so we can proceed?

@matjobst matjobst requested a review from Jegp September 21, 2023 13:44
@matjobst
Copy link
Collaborator Author

This is also a proposed fix to #36
The equality of all nodes is replaced by identity.

Copy link
Collaborator

@Jegp Jegp left a comment

Choose a reason for hiding this comment

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

I think this is a solid implementation and fulfills the specs. Nice work!

Please go ahead and squash + merge when ready :)

@matjobst matjobst merged commit 8c63f77 into main Oct 2, 2023
5 checks passed
@matjobst matjobst deleted the feature_inp_outp_properties branch October 2, 2023 23:40
matjobst added a commit that referenced this pull request Oct 16, 2023
* implemented .inputs and .outputs properties for NIRGraphs

* using identity for equality

* Fixed tests after rebase

* Added missing @DataClass(eq=False)
matjobst added a commit that referenced this pull request Oct 16, 2023
* implemented .inputs and .outputs properties for NIRGraphs

* using identity for equality

* Fixed tests after rebase

* Added missing @DataClass(eq=False)
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.

2 participants