Skip to content

Rename ArrayContainerT - #81

Merged
inducer merged 4 commits into
inducer:mainfrom
majosm:split-array-container-t
Aug 26, 2021
Merged

Rename ArrayContainerT#81
inducer merged 4 commits into
inducer:mainfrom
majosm:split-array-container-t

Conversation

@majosm

Copy link
Copy Markdown
Collaborator

Closes#80.

I noticed a couple of spots where it makes sense (I think) to require containers, so I also added a ContainerT in addition to ArrayOrContainerT.

@majosm

Copy link
Copy Markdown
CollaboratorAuthor

I had to remove a couple of the ContainerT usage instances to fix mypy failures, so it's maybe a little less useful than I thought. I don't suppose there's a way to indicate that a ContainerT is also an ArrayOrContainerT? I think that's (at least partially) what's throwing mypy off.

@majosm

Copy link
Copy Markdown
CollaboratorAuthor

I wonder if I could do something like:

ArrayT = TypeVar("ArrayT")
ContainerT = TypeVar("ContainerT")
ArrayOrContainerT = Union[ArrayT, ContainerT]

?

@majosm

Copy link
Copy Markdown
CollaboratorAuthor

That seems to work. Is it OK to add ArrayT? (Should I put it somewhere other than arraycontext.container?)

@majosm
majosm marked this pull request as ready for review August 26, 2021 15:47
@majosm
majosm requested review from alexfikl and inducerAugust 26, 2021 15:47

@alexfiklalexfikl left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm definitely on board with the renaming, since it's a lot clearer!

I'm not very convinced the type annotations for the two *_impl functions are correct though, mostly because they nest generic functions and reuse the same TypeVar. There are some rules about that in PEP 484, but they don't seem to mention this setup.

This was definitely an issue before this PR, so not really a showstopper, just thinking out loud.

Comment threadarraycontext/container/traversal.py Outdated
Co-authored-by: Alex Fikl <alexfikl@gmail.com>
@majosmmajosm mentioned this pull request Aug 26, 2021

@inducerinducer left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

LGTM. Thanks! I'll wait with merging until the CI issue from inducer/pytato#145 is done.

@inducer
inducer merged commit e08969f into inducer:mainAug 26, 2021
Sign up for freeto 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.

Meaning of ArrayContainerT

3 participants

@majosm@inducer@alexfikl