Skip to content

Conversation

@kaushikcfd
Copy link
Collaborator

@kaushikcfd kaushikcfd commented Oct 16, 2022

Pros:

  • Immutable types (frozen=True)
  • Derived classes can be dataclasses (noticed while implementing Functions #364)
  • In almost all cases uses the generated __init__.

Cons:

  • All derived classes must do eq=False in their call to dataclass, otherwise dataclasses will implement its own default which has a complexity of $O(2^N)$ in number of nodes in the DAG.
  • All derived classes must do def __hash__(self) -> int: return super().__hash__(), otherwise dataclasses will implement its own default.
  • Constructor arguments have to be _shape, _dtype instead of shape, dtype as pt.Array already has properties of that name.
  • ⚠️ Pylint Failures.

@kaushikcfd kaushikcfd force-pushed the fix_array_constructors branch 3 times, most recently from 5ca0093 to 291847c Compare October 16, 2022 16:44
@inducer
Copy link
Owner

inducer commented Oct 16, 2022

This generally seems worthwhile, I think.

  • All derived classes must do eq=False in their call to dataclass, otherwise dataclasses will implement its own default which has a complexity of O(2N) in number of nodes in the DAG.
  • All derived classes must do def __hash__(self) -> int: return super().__hash__(), otherwise dataclasses will implement its own default.

Is this something we could check for in __post_init__ in __debug__ mode?

pylint

Ah well. Maybe just disable unexpected-keyword-arg for pylint?

@kaushikcfd
Copy link
Collaborator Author

Ah well. Maybe just disable unexpected-keyword-arg for pylint?

Yep, I will do that for now. Related pylint issue: pylint-dev/pylint#7623.

@kaushikcfd kaushikcfd force-pushed the fix_array_constructors branch 3 times, most recently from e7921a8 to b63e2fb Compare October 16, 2022 21:47
@kaushikcfd
Copy link
Collaborator Author

kaushikcfd commented Oct 16, 2022

The pylint errors have been ignored. But the current refactoring depends on kw_only argument of dataclasses.field that was introduced in 3.10. Maybe we can merge this once we force the requirement to Py3.10?

@kaushikcfd
Copy link
Collaborator Author

Is this something we could check for in post_init in debug mode?

Yes, I think so. I will try to give it a shot.

@inducer
Copy link
Owner

inducer commented Oct 17, 2022

The pylint errors have been ignored. But the current refactoring depends on kw_only argument of dataclasses.field that was introduced in 3.10. Maybe we can merge this once we force the requirement to Py3.10?

Is there a way to avoid that? Requiring 3.10 is a healthy ways down the road in my view, given that we're not even completely switched over to 3.8.

@kaushikcfd
Copy link
Collaborator Author

Is there a way to avoid that?

We could use attrs instead of dataclasses?

@inducer
Copy link
Owner

inducer commented Oct 17, 2022

We could use attrs instead of dataclasses?

As long as it's easy enough to switch back to dataclasses, I would not be oppposed.

@kaushikcfd kaushikcfd force-pushed the fix_array_constructors branch from b63e2fb to af8eeff Compare October 17, 2022 04:38
@kaushikcfd kaushikcfd changed the title Use dataclasses for pt.Array Use <del>dataclasses</del> attrs for pt.Array Oct 17, 2022
@kaushikcfd kaushikcfd changed the title Use <del>dataclasses</del> attrs for pt.Array Use ~~dataclasses~~ attrs for pt.Array Oct 17, 2022
@kaushikcfd kaushikcfd force-pushed the fix_array_constructors branch from af8eeff to b3c35c5 Compare October 17, 2022 04:46
@kaushikcfd
Copy link
Collaborator Author

As long as it's easy enough to switch back to dataclasses, I would not be opposed.

Sure! It should take us an afternoon to switch to dataclasses from attrs.

@inducer inducer merged commit 1b5e773 into main Oct 17, 2022
@inducer inducer deleted the fix_array_constructors branch October 17, 2022 16:31
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.

3 participants