propagate NM in reducers #7
Conversation
after discussion with @tixxit, `NM` should have similar semantics to `NaN` in floating point arithmetic and so should propagate similarly.
| @@ -15,6 +15,8 @@ abstract class SimpleReducer[A: ClassTag, B] extends Reducer[A, B] { | |||
| val row = indices(i) | |||
| if (column.exists(row)) { | |||
| bldr += column.value(row) | |||
| } else if (column.missing(row) == NM) { | |||
| return NM | |||
There was a problem hiding this comment.
I don’t know if you feel that non-local return is abhorrent… I’m happy to rewrite the cfor explicitly as a while with the appropriate termination condition.
There was a problem hiding this comment.
As long as it isn't compiling down to a throw, I'm OK with returns where they make things clearer/faster :) I am unsure about using them in a cfor - cfor used rewrite to a tail-recursive method, but I think it is now a while. However, I wouldn't bank on that. May be worth rewriting as a while.
- the Mean reducer returns NM if it encounters an NM - fix tests accordingly
|
|
I'd say all of those should return NMs in the situation you mentioned. I mean, an NM may be a value, so the count won't be correct with an NM. Same for max/min. I think I'm going to fix up "Current" a bit in another PR. It doesn't need to be date specific. |
|
Actually, Current is a bit weird. I need to see where we are using it in content. It is basically Max, but uses only the first argument of a tuple and specializes it on LocalDate. So, we can just use Max, with an Order that does the same. So, I think Current should be removed entirely. |
Conflicts: modules/pframe/src/main/scala/pellucid/pframe/reduce/Mean.scala modules/pframe/src/main/scala/pellucid/pframe/reduce/SimpleReducer.scala
- the reducers returns NM if they encounter an NM - fix tests accordingly
|
Just remove it for now. It can literally be replaced with |
following discussion at #7 (comment) this reducer is equivalent to `Max(Order.by(_._1))`
after discussion with @tixxit,
NMshould have similar semantics toNaNin floating point arithmetic and so should propagate similarly.