Fast access

[SylvainCorlay]

A first take at the recursive version of access operators + moving the dimension checks up the call stack.

Trying to respect your code formatting conventions.

Closes #497 .

[dean0x7d]

On the topic of micro-optimizing the offset function, implementation 2 in the following code generates ideal assembly: https://godbolt.org/g/o2HC2G -- you might want to consider it.

[SylvainCorlay]

This actually looks really good.

[JohanMabille]

Indeed, we should use it in xtensor.

[aldanor]

This seems wrong? Note that the check here is for > whereas in other places it is for !=, otherwise it would have been implemented like you suggest here via a check_dimensions(...) helper.

[aldanor]

I guess I'd still prefer to have a separate byte_offset_unchecked() which would be used by all other methods (and xtensor), and having all other methods (including byte_offset()) be checked, so it's 'safe by default'.

[SylvainCorlay]

@aldanor @dean0x7d this version should be functionally equivalent and includes the fast proposal of Dean.

[SylvainCorlay]

So now byte_offset remains unsafe, but all calls to it in pybind call a check_dimensions first.

[aldanor]

Looks good.

#500 (comment) still stands though :)

How about this?

template<size_t dim = 0, typename... Ix> size_t byte_offset_unchecked(size_t i, Ix... index) const
{ /* current impl, checks nothing */ }

template<typename... Ix> size_t byte_offset(Ix... index) const {
    if (sizeof...(index) > ndim())
        fail_dim_check(sizeof...(index), "too many indices for an array");
    check_dimensions(index...);
    return byte_offset_unchecked(index...);
}

[SylvainCorlay]

ok, either that or byte_offset_safe?

[aldanor]

(the reasoning being: get_byte_offset() was previously part of private API; now byte_offset() is public so we make it safe by default)

[aldanor]

Ideally, I see unsafe version as one that's longer to type and that has a scary suffix like "unsafe" / "unchecked" / w/e, which also conveys the fact that it's "non-default". Since the difference is that it skips bounds checking, "_unchecked" makes sense, imo.

[SylvainCorlay]

I agree that any function callable directly from python should be safe and not crash the interpreter for any parameter value, but in the present case, accessors are only to be used from c++.

It is usual for c++ container to be unchecked (like std::vector::operator[])...

[aldanor]

Agreed. But we're trying to mirror ndarray behaviour here (to some extent, anyway...), hence the divergence.

[dean0x7d]

Least surprise: If most functions are checked by default, it's natural to assume they all are. Making just one silently unsafe will cause issues for users.

[SylvainCorlay]

I disagree with you guys, but I made the change here to get this part through.

I will open another issue for discussion on this point and something else that came up.

[aldanor]

Dimensionality check, too? (<)

[SylvainCorlay]

There was none originally.

[wjakob]

Shall we add one then?

[aldanor]

Btw: it's all quite trivial, but adding a test for byte_offset would be nice since it's public now.

[SylvainCorlay]

Btw, it is protected, not public.

[aldanor]

Ah, true, apologies.

All in all lgtm I think, great bikeshedding :)

[dean0x7d]

Sorry, I also missed the protected bit.

[aldanor]

Yea, viewing diffs on iPhone is hard, sorry :) To reiterate, the public API we really want to be safe by default, if it's protected then it could just stay unchecked of course. Or, you could have both like now and make the unsafe one public as well, whichever makes most sense.

[wjakob]

Btw: it's all quite trivial, but adding a test for byte_offset would be nice since it's public now.

👍

[SylvainCorlay]

@wjakob we cannot test this function externally because it is not public.

Only thing we could do in the test is inherit from it and expose a public function that calls it.

However this is already pretty much what offset_at` does. And it is already tested.

[wjakob]

Got it. Does it make sense to add a dimensionality test? (see the code review comments)

[SylvainCorlay]

There was not such check before. The current state of the PR is functionally equivalent to the previous one, but should be faster.

[SylvainCorlay]

Well my code is functionnally 5

On Nov 15, 2016 10:47 AM, "Wenzel Jakob" [email protected] wrote:

Got it. Does it make sense to add a dimensionality test (see the code
review comments)

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#500 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ACSXFn8JPa8CeStX8Xyx3Gf1tA1LREMLks5q-f4EgaJpZM4KyoPS
.

[wjakob]

Yes, but since you are already changing that code and moving tests around, it would be nice to fix this omission :)

[SylvainCorlay]

Also there are already checks for != and < (depending on the case) in the
calling code.

That would be duplicating the checks.

[SylvainCorlay]

Just realized this is not merged yet. Is there any blocker?

[wjakob]

What I don't understand is why the dimensionality check in offset_at (et al.) is not moved into check_dimensions?

[SylvainCorlay]

Because they differ depending of the function. Some check for dimension inequality and other for dimension mismatch. This was one of the items of the review by aldanor.

[wjakob]

Ok, I think I get it now -- thanks!

[SylvainCorlay]

Cool thanks!