-
Notifications
You must be signed in to change notification settings - Fork 0
Incorporating thrift, tweaking to get workflow working #82
Conversation
@@ -133,9 +130,6 @@ supplied, assumes a square matrix." | |||
([edge row col] | |||
(rowcol->idx edge edge row col)) | |||
([nrows ncols row col] | |||
{:pre [(not (or (neg? row), (>= row nrows) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These preconditions are added back later in a future pull request, slightly changed.
LGTM. #shipit |
(double (->> (for [[x weight] (partition 2 val-weight-pairs)] | ||
(if (>= weight 0) | ||
[(* x weight) weight] | ||
(throw-illegal "All weights must be positive."))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be a precondition instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What should it check for? We just need to be sure that the values coming out are doubles, right? They could come in as other things. @danhammer we ran into a bug for this. Can you remember why we switched this to (double ,,,,,)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, sorry, I thought weight was a parameter to the function. Actually though, do we need to throw an exception here? Like, if weight is greater than or equal to zero, are all bets off?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, if weights are < 0 all bets are off. I'll submit an issue so that we handle this in a precondition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eightysteele where are you at on this? |
Incorporating thrift in the utils namespaces, tweaking to get workflow working
Dan had to remove some pre/post conditions to get these things working with the workflow. I've opened an issue so that we can revisit this.