Skip to content
This repository was archived by the owner on Feb 17, 2023. It is now read-only.

Conversation

@rhattersley
Copy link
Member

aka. eq/ne for NumpyArrayAdapter and Array not hashable

@rhattersley rhattersley mentioned this pull request Jan 23, 2014
@shoyer
Copy link

shoyer commented Jan 23, 2014

👎 This does not mirror numpy, which treats == as a binary operation which returns another array. Baring good reasons otherwise, I would stick with how numpy does it.

Copy link
Member

Choose a reason for hiding this comment

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

This also has a different shape.

@rhattersley
Copy link
Member Author

👎 This does not mirror numpy, which treats == as a binary operation which returns another array.

Good point. I'm not sure yet what I think about this, so I'll share some background...

There are (at least in my head!) multiple layers to biggus. The first layer is the Array "interface" as currently defined and the various adapters, stacks, and mosaics that let one define a concrete instance. This layer just provides the bare minimum needed to describe virtual arrays, select sub-sections, and convert them to in-memory arrays. This is intended to be a generic building block.

The next layer is computation and is intended to be "optional" in some sense. This provides lazy evaluation of element-wise and aggregate operators, and is itself split into two layers. The first sub-layer allows for the construction of the lazy expression graphs. The second sub-layer provides the execution of the expression graphs, and is intended to be interchangeable. Different execution engines would target different underlying hardware platforms (e.g. SMP, SMP+GPU, cloud).

Matching this layered model, I've not defined any arithmetic operators on the Array class at all. For example, addition and subtraction are defined via biggus.add() and biggus.sub(). But, the allure of being able to write speed = sqrt(x*x + y*y) is undeniable!

@rhattersley
Copy link
Member Author

I've had a very useful chat with @bjlittle where we came up with alternative solution to the problem in SciTools/iris#900 which inspired this PR. Instead of pre-defining some concept of "equivalence" on the adapter class, we can just make public the underlying concrete array and leave the domain-specific "equivalence" logic to Iris.

As for defining operators on the Array class whilst keeping the code base tidy ... I'm sure it's doable, but I'll leave it for another day.

@pp-mo
Copy link
Member

pp-mo commented Jan 24, 2014

👍 from me, not much left to go wrong now ? ;-)
But probably best to sqwish for merge, as so much is stuff is otherwise introduced + reverted in the history.
I don't have merge rights, so I'll go + poke @bjlittle.

@rhattersley
Copy link
Member Author

Thanks @pp-mo - I've squashed as suggested.

@rhattersley
Copy link
Member Author

Instead of pre-defining some concept of "equivalence" on the adapter class, we can just make public the underlying concrete array and leave the domain-specific "equivalence" logic to Iris.

Done in #54.

@bjlittle
Copy link
Member

👍

bjlittle added a commit that referenced this pull request Jan 24, 2014
@bjlittle bjlittle merged commit 25b6bd4 into SciTools:master Jan 24, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants