rainwoodman / vast

vala and scientific numerical computation
11 stars 1 forks source link

move logic for manipulating strides to ViewBuilder. #34

Closed rainwoodman closed 7 years ago

arteymix commented 7 years ago

I wonder whether we should pass the axis to slice. Maybe have a counter everytime we slice?

arr.view ()
   .slice (0, size_t.MAX) // axis 0
   .slice (0, size_t.MAX) // axis 1
   .skip ()     // skip axis 2
   .end ();
rainwoodman commented 7 years ago

No. not a counter -- they are intended to be stateless -- because we need to be able to call this api in a loop or selectively apply them on some axes.

arteymix commented 7 years ago

Then maybe a axis selector that would use the default axis if not specified:

a.view ()
    .axis (0).slice (1, 5)
    .axis (1).slice (4, 9)
    .end ();

Let's say that a buch of number is hardly readable for a builder.

arteymix commented 7 years ago

To remain consistent with the other APIs, we could have:

rainwoodman commented 7 years ago

Indeed it is hardly readable, but vala doesn't support named arguments -- it is a defect in the language.

I strongly prefer not having selectors -- it adds in a state to the ViewBuilder and thus reminds me cin / cout of C++. The formatter are uttly verbose.

The StringBuilder in vala is much nicer in comparison.

rainwoodman commented 7 years ago

The idea of the ViewBuilder is to have a mechanism to implement higher level operations in the array, not to implement the higher level operations internally, so these methods shall stay in Array as convienent wrappers to .view(), in my opinion.

rainwoodman commented 7 years ago

I originally put step before slice because the default value of from and to are realized using the value of step. I think the tradition is having step later. My brain short-circuited. I'll switch the order.

arteymix commented 7 years ago

I agree on the necessity of such feature: it's not convenient to work on individual axis otherwise.

Maybe at least introduce head and tail to avoid using arbitrary constants?

rainwoodman commented 7 years ago

To be complete, we'll need four functions to cover each of these:

a:b:c (slice)
:b:c (tail)
::c (step)
a::c (head)

(because default value of c is 1, in the domain of ssize_t)

I really would rather having just one function... Do you have a reference removing ssize_t.MAX from the domain causes problem in real world?

rainwoodman commented 7 years ago

I sense that as long as we don't poke a hole (thus we shall remove from the edge of the domain, e.g. ssize_t.MAX) it won't be mathmatically uglier than the original definition of ssize_t.

arteymix commented 7 years ago

I feel like it's a very dirty hack. Normally, that would cause an out-of-bound array access.

We're not in Python here, so we have to be consequent.

Also, I don't like the word View: our arrays are basically all views over a GBytes object. Thus, I think Array.Builder would suffice.

Sorry for being so negative, I only mean best for this project and designing good APIs is important.

rainwoodman commented 7 years ago

I have a uglier proposal what about

.slice(0, null, {0}, {1})

null is THRU. problem solved.

arteymix commented 7 years ago

Now C users have to declare in advance a size_t* pointer before passing it. Honestly, stop resisting! Python bindings will have the nice slicing protocol anyway!

arteymix commented 7 years ago

Can we have a multi-dimensional broadcast utility as well? We might also need a way of finding a broadcasting shape for multiple arrays, possibly into MultiIterator.

rainwoodman commented 7 years ago

OK -- four functions on the go.

That way of finding shape shall be somewhere else -- not here then a follow up function makes use of Builder to create broadcasted views.

arteymix commented 7 years ago

I'm going to sleep. I'll review all this tomorrow if you want. It sounds really good and elegant to me.

rainwoodman commented 7 years ago

OK. I added a qslice that covers all four cases with pointer promotion. There is a lot of code duplication and I do prefer not to refactor them into really short functions -- not for performance but for readability.

rainwoodman commented 7 years ago

Ok, the best of both worlds. we have five functions. (head, tail, step, slice -- for c binding) all calls qslice (internal, vala, python, ...).

arteymix commented 7 years ago

For qslice, just using ssize_t? is sufficient to promote the type to a pointer.

It really looks nice to me :+1:.

What about reverse slicing? The reason I added it in Array.slice is that it was not dealing with step as well, so it made sense to use negative step if the slice is reversed.

arteymix commented 7 years ago

qslice could be named slice, but use a different cname. This way we would have both slice for C and Vala but with slightly different semantics, yet an uniform API.

arteymix commented 7 years ago

That said, if we want Vala-specific APIs, we're better adding a vast-1.0-custom.vapi and put the code literally there. It will avoid polluting the symbol space with unusable functions from C.

arteymix commented 7 years ago

I'm really glad of these changes, it will make axis manipulation a breeze ;) I don't think NumPy has anything similar.

rainwoodman commented 7 years ago

oh man conflict...

rainwoodman commented 7 years ago

The nullable ssize_t is neat look at the test case -- do we still need the head tail methods?

The original concern regarding requiring a pointer syntax in C is not that bad -- most of the cases the parameters are passed in as an addressable variable, not a number literal anyways. Ocacionally we do need use literals there must be a macro to make it addressable, e.g., in c99,

#define _A(a) (gssize_t []) {a}
arteymix commented 7 years ago

I still prefer head and tail though because it does not involve any trick. If you are really to slice from and to an edge, it's better being explicit, no?

Maybe we can try to patch Vala slice protocol to include [a:] and [:b] notation? and also stepping?

arteymix commented 7 years ago

If you want I can merge the PR and deal with conflicts. I think I'll have to rewrite a few utilities I added recently, but that's okay.

rainwoodman commented 7 years ago

On the other hand, I was already able to make mistakes translating the slice to head/tail.

I pondered how and realized confusion is that

So it is fundamentally a pair of mind twisters -- take time to get used to and may come back and bite you later.

rainwoodman commented 7 years ago

yes. Let's merge this and talk about vala patch later.

arteymix commented 7 years ago

Okay, good, I'll merge this locally then.

arteymix commented 7 years ago

Ok, it seems like you've already rebased everything. I leave you the honor to merge!