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

lvalue semantic for diag #161

Open
SylvainCorlay opened this issue Mar 9, 2017 · 10 comments
Open

lvalue semantic for diag #161

SylvainCorlay opened this issue Mar 9, 2017 · 10 comments

Comments

@SylvainCorlay
Copy link
Member

It would make sense for diag to return an assignable view.

@JohanMabille
Copy link
Member

In that case let's do the same for triu, tril, and flipud/flipr. This has be mentioned in #158.

@SylvainCorlay
Copy link
Member Author

SylvainCorlay commented Mar 9, 2017

The flip versions should be doable with views ans dynamic views eventually as mentioned by @wolfv in the original implementation.

Diag probably requires a specific view.

@wolfv
Copy link
Member

wolfv commented Mar 9, 2017

But diag is actually not a view in NumPy.
To be honest I don't even know if it is a good idea to make the flips views. This might be just an artifact on how it's implemented in NumPy. I wouldn't really expect the source array to change after assigning something to the flipped thing on a NumPy array.

@SylvainCorlay
Copy link
Member Author

SylvainCorlay commented Mar 9, 2017 via email

@wolfv
Copy link
Member

wolfv commented Mar 9, 2017

But thinking about it, the case of diag is really quite unclear, isn't it?

For example:

a = xarray({1,2,3});
b = diag(a);
// = {{1, 0, 0}, {0, 2, 0}, {0, 0, 3}};
b += 123;
// = {{124, 123, 123}, ...
// and now a = {{124, 125, 126}}?

Or did you mean diagonal? I agree, for diagonal it makes a lot of sense, even though NumPy doesn't seem to allow it.

@SylvainCorlay
Copy link
Member Author

Yes I meant diagonal.

@wolfv
Copy link
Member

wolfv commented Mar 15, 2017

by the way, the numpy docs mention

In some future release, it will return a read/write view and writing to the returned array will alter your original array. The returned array will have the same type as the input array.

So I guess it would be a good move to overtake NumPy on this feature! :)

@SylvainCorlay
Copy link
Member Author

Good catch.

@wolfv
Copy link
Member

wolfv commented Mar 23, 2017

This was an experiment hijacking the xindexview (and using a functor instead of a container).

However, the xindexview currently can only have a 1D shape, so the more advanced diags don't work.

    struct diag_indices
    {
        diag_indices(std::size_t sz) : m_size(sz)
        {
        }

        template <class... Args>
        xindex operator() (std::size_t idx, Args... args) {
            return xindex(idx, idx);
        }

        xindex operator[](std::size_t idx) const
        {
            return xindex({idx, idx});
        }

        std::size_t size() const {
            return m_size;
        }
    private:
        std::size_t m_size;
    };

    template <class E>
    inline auto diagview(E&& e) noexcept
    {
        using view_type = xindexview<xclosure_t<E>, diag_indices>;
        auto sz = e.shape()[1];
        return view_type(std::forward<E>(e), diag_indices(sz));
    }

@wolfv
Copy link
Member

wolfv commented Mar 23, 2017

Maybe the proposed functor view could also be used for diagonal?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants