Refactor for clarity, add comments. #57
Conversation
matthew-brett
force-pushed
the
refactor-algorithm
branch
from
dc0581c to
059b0f4
Compare
|
For the tests - I wasn't sure what to do. I think the original tests are assuming that something like the following will always give the same output value # In the test function.
np.random.seed(2025)
# In `select_proposals`
np.random.seed(None)
i = np.random.choice(1000) # e.g.but in my hands, that is not true. Accordingly the tests on |
We were confused by the algorithm, and this seemed generally undesirable, so we have attempted a refactor for greater clarity. We've also used the more standard RNG instance pattern for the random number generation. The tests are failing, but this seems also to be true of `main`.
matthew-brett
force-pushed
the
refactor-algorithm
branch
from
059b0f4 to
e43b361
Compare
|
@lucascolley - what say you for the tests? |
|
#57 (comment) sounds good to me. What is the diff to make the tests pass? https://github.com/15r10nk/inline-snapshot could make that easier to find. |
This makes it easier to compare expected to actual test output.
|
OK - I've added some tests to assert the proportions match the weights, and updated the test output to the current test output. I did a little more refactoring of the project random selection for clarity. I've also added a |
Although doing it in one go is neater, perhaps it will delay review by forcing reviewers to confirm it gives the same output.
|
I've reverted the algorithm to the stepwise selection procedure; ordering and then confirming is neater to my eye, but I thought it would require extra review work to confirm it gives equivalent results. I did use |
matthew-brett
force-pushed
the
refactor-algorithm
branch
from
60d52f6 to
848c0a6
Compare
2 participants
We were finding it hard to follow the algorithm, and this seemed generally undesirable, so we have attempted a refactor for greater clarity.
We've also used the more standard RNG instance pattern for the random number generation.
The tests are failing, but this seems also to be true of
main.