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

Numpy array context rebased #190

Merged
merged 32 commits into from
Sep 4, 2024
Merged

Numpy array context rebased #190

merged 32 commits into from
Sep 4, 2024

Conversation

inducer
Copy link
Owner

@inducer inducer commented Sep 1, 2022

This does a fair bit of stuff. The main components are a numpy array context and an in-depth rework of dataclass arraycontainer broadcasting. There is also a tweak to array container serialization ("Container serialization: iterable -> sequence, plus type aliases") that makes the serialized container a sequence and introduces a type alias for it. I've flagged some things that I'm less sure about in a self-review below.

The whole thing is probably best read commit-by-commit to avoid mixing changes that belong to different topic areas.

Closes #93
Closes #235
Closes #95

TODO

  • Are np.minimum, np.maximum missing?

@inducer inducer changed the title #93 rebased Numpy array context rebased Sep 5, 2022
@inducer inducer force-pushed the numpy-rebased branch 5 times, most recently from d2257ef to 03186f4 Compare August 1, 2024 15:05
arraycontext/context.py Show resolved Hide resolved
arraycontext/container/arithmetic.py Outdated Show resolved Hide resolved
arraycontext/container/arithmetic.py Outdated Show resolved Hide resolved
arraycontext/container/arithmetic.py Show resolved Hide resolved
arraycontext/container/arithmetic.py Show resolved Hide resolved
arraycontext/container/arithmetic.py Show resolved Hide resolved
arraycontext/container/arithmetic.py Outdated Show resolved Hide resolved
arraycontext/container/arithmetic.py Show resolved Hide resolved
arraycontext/container/__init__.py Show resolved Hide resolved
arraycontext/impl/jax/fake_numpy.py Show resolved Hide resolved
@inducer inducer marked this pull request as ready for review August 1, 2024 15:08
@inducer
Copy link
Owner Author

inducer commented Aug 1, 2024

Hi all, please take a look, particularly with an eye towards how much you like the direction this is going. It should not break compatibility right now, but it contains some things that will need changing once we remove what's being deprecated now.

@inducer
Copy link
Owner Author

inducer commented Aug 1, 2024

Also, sorry about the breakage from my last-minute changes, I'll fix that right away.

Copy link
Collaborator

@alexfikl alexfikl left a comment

Choose a reason for hiding this comment

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

Went through the easy parts of this and left a bunch of nitpicks. Will have to look at the arithmetic changes a bit more carefully some other time though.

From the tests, the Bcast syntax seems a bit clunky with all the wrapping. Maybe we can introduce some helper functions?

result = broadcast(operator.op, container1, container2, level_or_actx=1)

? Will ponder a bit 😁

.github/workflows/ci.yml Outdated Show resolved Hide resolved
arraycontext/container/__init__.py Show resolved Hide resolved
arraycontext/container/__init__.py Outdated Show resolved Hide resolved
arraycontext/impl/numpy/__init__.py Outdated Show resolved Hide resolved
arraycontext/impl/numpy/__init__.py Outdated Show resolved Hide resolved
arraycontext/impl/numpy/__init__.py Outdated Show resolved Hide resolved
arraycontext/impl/numpy/__init__.py Show resolved Hide resolved
arraycontext/impl/numpy/__init__.py Show resolved Hide resolved
arraycontext/impl/numpy/fake_numpy.py Outdated Show resolved Hide resolved
arraycontext/impl/numpy/fake_numpy.py Outdated Show resolved Hide resolved
@alexfikl
Copy link
Collaborator

alexfikl commented Aug 1, 2024

Was just googling around and found a jax pytree version of container math:
https://github.com/google/tree-math
It seems to just allow broadcasting scalars at the leaf level, so it's a lot smaller in scope.

@alexfikl
Copy link
Collaborator

alexfikl commented Aug 5, 2024

Following a meeting with @inducer on Friday, he suggested I write down any random thoughts here, so that we can discuss. So here goes before I forget again!

My question was mostly related to what broadcasting we actually need to do? The broadcasting implementation here seems to be very general and very flexible (and quite explicit as well).

Having experience with the pytential part of the stack (as opposed to grudge), these operations seem to be required:

  1. Just direct adding of two DOFArrays (no broadcasting here), e.g. x + y.
    • This should just work no questions asked.
  2. Broadcast DOFArray with a scalar (of some sort? device or host), e.g. x + 1.
    • Can work with a rec_[multi]map_array_container, but having the overload work is nicer.
  3. Broadcast a numpy object array (of scalars or of DOFArray) with a DOFArray, e.g. velocity - mean.
    • This should also work with a [multi]map_array_container, but having the overload work is nicer. Object arrays already broadcast constants, so that will just work.

i.e. all those could be handled (maybe a bit verbosely) without any broadcasting support in arraycontext.with_container_arithmetic. Broadcasting a scalar seems like a common enough case to implement here though.

What fancier broadcasting would we need to support? I guess mirgecom has the most advanced needs here.

@inducer inducer force-pushed the numpy-rebased branch 5 times, most recently from f278400 to e1f678a Compare August 5, 2024 17:14
@inducer inducer mentioned this pull request Aug 5, 2024
@inducer inducer force-pushed the numpy-rebased branch 2 times, most recently from 7ac0e4e to 6251b61 Compare August 25, 2024 21:42
@inducer
Copy link
Owner Author

inducer commented Aug 25, 2024

Alright, Bcast is gone from this, now in #280 should we need it.

Copy link
Collaborator

@alexfikl alexfikl left a comment

Choose a reason for hiding this comment

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

Without the new broadcasting interface it's mostly straightforward changes, so this looks good to me!

Went through the arithmetic changes as well, but they seem to mostly be handling the wealth of options and I couldn't spot anything fishy there.

arraycontext/container/__init__.py Show resolved Hide resolved
arraycontext/impl/numpy/__init__.py Show resolved Hide resolved
arraycontext/impl/numpy/__init__.py Outdated Show resolved Hide resolved
arraycontext/impl/numpy/__init__.py Outdated Show resolved Hide resolved
@inducer inducer mentioned this pull request Aug 31, 2024
@inducer inducer force-pushed the numpy-rebased branch 2 times, most recently from ce9736c to dfc08cd Compare September 4, 2024 20:04
@inducer inducer enabled auto-merge (rebase) September 4, 2024 20:19
@inducer inducer merged commit 9b75590 into main Sep 4, 2024
12 checks passed
@inducer inducer deleted the numpy-rebased branch September 4, 2024 20: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.

5 participants