Correct choices function#299
Conversation
The function was returning invalid results due to referencing the wrong data structure. Some additional logic is now also included to make sure that the operation behaves a little more efficiently. `MetricSpace` did not use the memoization scheme correctly, resulting in no benefit actually being present. This is now correct, referencing the memoization map structure.
KyleErwin
left a comment
KyleErwin
left a comment
There was a problem hiding this comment.
Just a comment on choices as a whole, should we not cater to cases where n is the greater than the list.size?
| innerDropIndex(0, l) | ||
| } | ||
|
|
||
| def go(indexes: List[Int], current: List[A]): List[A] = |
There was a problem hiding this comment.
What happens if, for example we have a List of size n and we generate random indices such as n-2 and n-1. And since the method go will remove indices already visited then the index n-1 will become invalid. Is this correct?
There was a problem hiding this comment.
I don't follow your thinking here. The values in backsaw are based on a "reducing list size", so even if the same same value is randomly selected, the selected element will be within range and the elements will always be shrinking as selections take place.
There was a problem hiding this comment.
I'm with you now. That's very cool. One final thought though, because backsaw uses the reducing list size to generate random indices do we not end up giving preference to indices 0 to n - 1 as they would have the most opportunities to be generated?
There was a problem hiding this comment.
I understand now that since already visited elements are being removed that the elements closer to xs.length will also get a chance of selected fairly. Will Approve the PR
| case _ => List.empty[A] | ||
| } | ||
|
|
||
| go(l, xs.toList).some |
There was a problem hiding this comment.
Does choices now eliminate any duplicates generated?
There was a problem hiding this comment.
The tests should be illustrating that fairly well.
There was a problem hiding this comment.
Cool, I see that. Was just double checking.
|
What would the correct behaviour be? Request |
|
Was just wondering if RVar.choose(NonEmptyList(...)).replicateM(n) |
|
Yeah, I would be very hesitant to overload the behaviour like that. It seems very out of the functions scope. |
2 participants
The function was returning invalid results due to referencing the
wrong data structure. Some additional logic is now also included to
make sure that the operation behaves a little more efficiently.
MetricSpacedid not use the memoization scheme correctly, resultingin no benefit actually being present. This is now correct, referencing
the memoization map structure.