-
Notifications
You must be signed in to change notification settings - Fork 11
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
Conversation
d2257ef
to
03186f4
Compare
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. |
Also, sorry about the breakage from my last-minute changes, I'll fix that right away. |
There was a problem hiding this 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 😁
Was just googling around and found a jax pytree version of container math: |
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
i.e. all those could be handled (maybe a bit verbosely) without any broadcasting support in What fancier broadcasting would we need to support? I guess |
f278400
to
e1f678a
Compare
7ac0e4e
to
6251b61
Compare
Alright, |
There was a problem hiding this 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.
ce9736c
to
dfc08cd
Compare
- Deprecate automatic broadcasting of array context arrays - Warn about uses of numpy array broadcasting, deprecated earlier - Clarify documentation, warning wording
…across who Names suggested by @majosm
dfc08cd
to
bc323fc
Compare
This does a fair bit of stuff. The main components are a
numpy
array context and an in-depth rework ofdataclass
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
np.minimum
,np.maximum
missing?