Broadcasting Proposal

[stuartlynn]

Hey @kylebarron putting this here for discussion. I think this might be a good pattern for functions that can take either a value or a series as their arguments.

I have implemented it for rotate so we can get a feel of how it works.

Will move the BroadcastableParameter enum to its own file if we are happy with this approach

• Defines an BroadcastableParameter enum
• Implements Into<> for BroadcastableParameter, primitive types + Series references
• adapts the rotate function to take impl Into<BroadcastableParameter>
• apply rotation logic based on the vale or series passed to the function

[kylebarron]

I'm still wrapping my head around lifetimes, so I might have a couple dumb questions.

Here we need a lifetime on the enum because the enum takes a reference to the series right? And otherwise the pointer from the enum to the series could outlive the memory contained by the series?

If a function takes a &Series as an input parameter directly, would it still need a lifetime? Or no because the series wouldn't be stored inside an object like an enum?

[stuartlynn]

Still pretty new to me to but yeah I believe that's the case. When passed a &Series object directly, I think the compiler is able to infer the lifetime for the parameter given the calling context and the function that it is passed to. You might still get a lifetime warning if you do something in that function, like assign the pointer to a property of another struct that you return for instance, but most of the time the compiler is able to infer the lifetime

[kylebarron]

I guess in the future if we had lots of different types of scalar parameters we could make a macro here, but for this many it's definitely clearer without a macro

[stuartlynn]

Yeah for sure. I tried to implement this using trait bounds, for example with the num crates ToPrimative (https://docs.rs/num/latest/num/trait.ToPrimitive.html) you can do something like:

impl<'a> From<T> for BroadcastParameter<'a,T>
  where T : impl ToPrimative
{
...
}

impl<'a> From<Series> for BroadcastParameter<'a,Series>
{
...
}

The problem is that the compiler disallows this because we dont own the Series struct and the owner of that Struct might in a future release implement the trait ToPrimative for Series, breaking the code above.

So we can either be explicit or create a macro.
but there is an issue with then also implementing it for Series as it's from a foreign crate

[kylebarron]

Maybe another dumb question but how does this work with BroadcastableParameter<'a, f32>, when you only defined From on f64?

[stuartlynn]

Oh I hadn't noticed that myself, I think I had probably meant to put f64 there. Presumably it works because f32 can be coerced in to an f64? If we had defined it the other way around, implemented the From for f32 and try a impl Into<BroadcastableParameter<'a,f64> it would have failed

[kylebarron]

I feel like this could get messy quickly, where we have three different enum values for TransformOrigin and two different enum values for BroadcastableParameter, though I don't immediately have a better idea

[kylebarron]

Unrelated: At some point we should get more serious about error handling and ban .unwrap and .expect, but fine for now

[kylebarron]

One question here: if we're now taking series as input, then how should we handle its inherent dynamic typing? Here the code would panic if the series contained any data type other than f64. What if the series contained f32...? Seems like a situation where we'd want several numeric data types to work seamlessly

[kylebarron]

Is the extra { after Box::new( intentional?

Why do you have move here, when you don't in the two cases above?

[stuartlynn]

Updated this to allow the Broadcastable parameters to be iterated over. If they are a series, we return the next value in the series iterator and if they are a single value we just return that value over and over again.

This lets us use them in a cleaner way when there are multiple potential broadcastable parameters passed to a function, we can just zip them with the series we are operating on and use the values regardless. For example

let angle: BroadcastableParameter<'a, f64> = angle.into();

let rotated_geoms: Vec<Geometry<f64>> = iter_geom(self)
           .zip(angle.into_iter())
           .map(|(geom, angle)| apply_rotation(&geom, angle.extract::<f64>().unwrap()))
           .collect();

The iterator returns an AnyValue to be consistent with the output of SeriesIter from polars. We will want to make that unwrap actually return a Result if the type cant be converted to one we need. That will require making these functions also return a result .

Next up is to try and remove the f64 type from the BroadcastableParameter and just have it be anything that AnyType supports. Having issues with this kind of bug when trying to do this with generics and Impls though. For example:

impl <'a,T> From<T> for BroadcastableParameter<'a>
    where T : Into<AnyValue<'a>>
{
    fn from(val: T) -> Self{
        BroadcastableParameter::Val(val.into())
    }
}


impl<'a> From<&'a Series> for BroadcastableParameter<'a>
{
    fn from(series: &'a Series) -> BroadcastableParameter {
        BroadcastableParameter::Series(series)
    }
}

Gives the error

error[E0119]: conflicting implementations of trait `std::convert::From<&polars::prelude::Series>` for type `broadcasting::BroadcastableParameter<'_>`
  --> geopolars/src/broadcasting.rs:55:1
   |
46 | / impl <'a,T> From<T> for BroadcastableParameter<'a>
47 | |     where T : Into<AnyValue<'a>>
48 | | {
49 | |     fn from(val: T) -> Self{
50 | |         BroadcastableParameter::Val(val.into())
51 | |     }
52 | | }
   | |_- first implementation here
...
55 | / impl<'a> From<&'a Series> for BroadcastableParameter<'a>
56 | | where
57 | | {
58 | |     fn from(series: &'a Series) -> BroadcastableParameter {
59 | |         BroadcastableParameter::Series(series)
60 | |     }
61 | | }
   | |_^ conflicting implementation for `broadcasting::BroadcastableParameter<'_>`
   |
   = note: upstream crates may add a new impl of trait `std::convert::From<&polars::prelude::Series>` for type `polars::prelude::AnyValue<'_>` in future versions

which seems like it's hard to get around.
https://stackoverflow.com/questions/63136970/how-do-i-work-around-the-upstream-crates-may-add-a-new-impl-of-trait-error

[kylebarron]

If they are a series, we return the next value in the series iterator and if they are a single value we just return that value over and over again.

One worry is that this won't error if the series argument isn't the same size as the underlying series data? Seems like we'd want to assert that. But I agree it's clean.

conflicting implementation for

Ah, my favorite error. I hit this (and haven't figured out a solution) when trying to update geo algorithms to use traits as input. I assume we won't be able to define From on two types we don't own, like your error suggests.