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

Allow &a @ &b to have different lifetimes #103

Merged
merged 2 commits into from
Mar 1, 2016

Conversation

daniel-vainsencher
Copy link
Contributor

Like it says, an about 5 character diff. Logic is that both operands are being read only, so there is no need on any lifetime constraint, in particular they do not need have the same lifetime.

@bluss
Copy link
Member

bluss commented Mar 1, 2016

How did you run into this? If you have two & from different scopes rust should infer their intersection lifetime automatically.

@daniel-vainsencher
Copy link
Contributor Author

Really? I wasn't expecting it to. I just put up the commit that creates this problem for me, on rustc 1.8.0-nightly (d5558825b 2016-02-28).

It is daniel-vainsencher/online_weighted_stats@8f9cf03

BTW, I have found porting that code to ndarray quite pleasant, other than the issue we are discussing. The scalar support worked well. I don't enjoy the iadd style, but it is a good compromise until @= are widely available. I think I'll be porting a much bigger project soon.

@bluss
Copy link
Member

bluss commented Mar 1, 2016

Thank you for sharing the code (I did compile it).

This surprises me. &*data - &self.mean compiles, but that shouldn't be necessary...

Anyway, the primitive types do the same thing (different lifetime parameters) http://doc.rust-lang.org/nightly/std/primitive.f64.html so it seems like the right thing to do!

For future reference, rebased PRs are preferable to merge (when possible).

Great to hear your feedback. @= is stable in Rust 1.8 (it was just decided), so it's only 6 weeks left! 😄

I am continuously tweaking some details related to trait bounds on array elements and scalars for arithmetic operators. We can't really get everything we want, there's two reasons.

  • Rust does not have specialization yet. So we use Any for ad-hoc specialization where it is needed (transparent BLAS use will need this)
  • Some trait coherence issues mean that ScalarOperand is a needed trait. Simply a marker so that the type system knows for array + x whether to pick the array or the scalar addition.

bluss added a commit that referenced this pull request Mar 1, 2016
Allow &a @ &b to have different lifetimes
@bluss bluss merged commit f0628c3 into rust-ndarray:master Mar 1, 2016
@bluss
Copy link
Member

bluss commented Mar 1, 2016

By the way, we want to make it easier to do multiple arithmetic operations without needing to create a temporary array like happens here.

Like the issue #88 points out, it's already possible manually to use .zip_mut_with, but I think we want some convenience methods here. The space is open to creativity though, I don't know what you think.

Even this could be possible: array1 += Product(alpha, &array2);, but since it involves implementing the arithmetic operators again for all operators, for all array types for a new type Product, I don't think it's a good idea.

@daniel-vainsencher
Copy link
Contributor Author

I think the lifetime behavior + error msg that commit gets in rustc was certainly confusing. What should I file a bug against?

https://github.com/rust-lang/rust?

@bluss
Copy link
Member

bluss commented Mar 1, 2016

Yes

@daniel-vainsencher
Copy link
Contributor Author

There seem to be two natural types of interpretation for the same
expressions. The current is very local hence predictable: each operator is
interpreted on its own. The downside is fewer opportunities for
optimization. These compose in the sense that they create arrays of
compatible types. Call this base level.

The other to my understanding, is creating expression trees that compose.
These can then be interpreted globally (at compile- or runtime) to plan the
evaluation of the whole expression. For example, if multiplying A_B_C
where A is larger than B,C, we might exploit associativity to defer
broadcasting (multiply each B by its correspond C just once), and obviously
not create more temp vars than needed. In C++, I understand this kind of
interpretation is done at compile time via expression template libraries.
Call this expression level.

This raises some issues:

  • Base level is easy to debug, hence valuable anyway. For me at least,
    short term, having this be close to math notation is the most important
    aspect: this makes algorithms easy to maintain, and I can usually restrict
    the dirty save every flop tricks for a small subset of expressions.
  • foe the wxpression level, do you mean for interpretation to happen at
    compile or run time?
  • What relevant mechanisms are available in rust for interpreting an
    expression at compile time? Macros obviously. Maybe some type level
    computation plus specialization?
  • What set of optimizations would be reliably beneficial? An advantage for
    runtime planning is we can measure what works better...
  • what is the surface syntax for using the expression level? I would
    prefer it still look a lot like math notation. Perhaps ideal is to have a
    separate type for deferred/logical arrays, so that instead of Product we
    can still use *.
    On Mar 1, 2016 9:59 AM, "bluss" notifications@github.com wrote:

Yes


Reply to this email directly or view it on GitHub
#103 (comment).

@daniel-vainsencher
Copy link
Contributor Author

Sorry, just realized you probably don't mean anything as complicated as I was referring to.

Anyway, a question. In commit [1], in addition to working around the lifetime issue, I also added some to_owned's in a test, to comply with the signature I chose for next_value. What type/trait bound should I give data and/or self.mean in order to work with any reasonable kind of ndarray? I only read their content, so I really don't care whether I get an owned, shared, view or whatever storage strategy. This is a common case for me at least.

[1] daniel-vainsencher/online_weighted_stats@0bfc04d

@bluss
Copy link
Member

bluss commented Mar 2, 2016

How to design it in Rust an open ended question, so any complicated scheme is on topc. It may even be a major thing enough that it is best explored outside of ndarray.

Rust has a pretty advanced type system too, but I don't know how well expression templates work out.

About the question. Note that arr1 has changed to return OwnedArray in the 0.4 alphas. I'm still gradually shifting the crate's API from the old version that only had RcArray.

The simplest way to work with any array is to accept an array view (requires caller to convert to a view if needed). Apart from that, I'm not sure what to recommend, this is not fully fleshed out in the library. Accepting different array types right now requires using the Data.. etc traits which is a bit complicated, I hope we can do something better in the long run.

@daniel-vainsencher
Copy link
Contributor Author

I this requiring callers to convert into views is not great, ergonomically,
it can lead to lots of conversions in the middle of algorithms. Is there an
issue open for this?

Maybe we can use something like Deref? if you squint, any *Array points at
a view on its data. Or implement From for references to different *Array
types into an array view, and require an Into trait bound, then
convert at the beginning of each function.

On Wed, Mar 2, 2016 at 8:46 AM, bluss notifications@github.com wrote:

How to design it in Rust an open ended question, so any complicated scheme
is on topc. It may even be a major thing enough that it is best explored
outside of ndarray.

Rust has a pretty advanced type system too, but I don't know how well
expression templates work out.

About the question. Note that arr1 has changed to return OwnedArray in
the 0.4 alphas. I'm still gradually shifting the crate's API from the old
version that only had RcArray.

The simplest way to work with any array is to accept an array view
(requires caller to convert to a view if needed). Apart from that, I'm not
sure what to recommend, this is not fully fleshed out in the library.
Accepting different array types right now requires using the Data.. etc
traits which is a bit complicated, I hope we can do something better in the
long run.


Reply to this email directly or view it on GitHub
#103 (comment).

Daniel Vainsencher

@bluss
Copy link
Member

bluss commented Mar 2, 2016

We should create an issue for that. We can't Deref directly to ArrayView for rust reasons: We must return a &T from .deref(), we can't create a new value. Because of this, right now types like ArrayView<'a, T, Ix> are second class compared to real view types built into the language like &'a [T].

Introducing views was still a huge boon to the library. We can now do sensible things like mutable subviews, a mutable diagonal view, etc.

Maybe it's possible to deref to something, or we can declare a new trait for all arrays, and we will also consider the Into approach.

jturner314 added a commit to jturner314/ndarray that referenced this pull request Nov 23, 2017
Rust 1.22 fixes the original issue (rust-ndarray#103) that required `'b` to be
added. (See rust-lang/rust#32008 and rust-lang/rust#45435 for the
issue and fix in Rust.)
jturner314 added a commit to jturner314/ndarray that referenced this pull request May 10, 2018
Rust 1.23 fixes the original issue (rust-ndarray#103) that required `'b` to be
added. (See rust-lang/rust#32008 and rust-lang/rust#45435 for the
issue and fix in Rust.)
jturner314 added a commit to jturner314/ndarray that referenced this pull request May 10, 2018
Rust 1.23 fixed the original issue (rust-ndarray#103) that required `'b` to be
added. (See rust-lang/rust#32008 and rust-lang/rust#45435 for the
issue and fix in Rust.)
jturner314 added a commit to jturner314/ndarray that referenced this pull request May 10, 2018
Rust 1.23 fixed the original issue (rust-ndarray#103) that required `'b` to be
added. (See rust-lang/rust#32008, rust-lang/rust#45425, and
rust-lang/rust#45435 for the relevant Rust issues/PRs.)
jturner314 added a commit to jturner314/ndarray that referenced this pull request Jun 13, 2018
Rust 1.23 fixed the original issue (rust-ndarray#103) that required `'b` to be
added. (See rust-lang/rust#32008, rust-lang/rust#45425, and
rust-lang/rust#45435 for the relevant Rust issues/PRs.)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants