Skip to content

SMS simplification#87

Open
nboullis wants to merge 13 commits intodevelopfrom
SMS_adjust
Open

SMS simplification#87
nboullis wants to merge 13 commits intodevelopfrom
SMS_adjust

Conversation

@nboullis
Copy link
Collaborator

@nboullis nboullis commented May 13, 2025

I did my best to simplify the code for the SMS movement.

As I understand it, looping until finding a suitable next cell was only needed because the computed probabilities did not sum up to 1.

I fixed this and added a bunch of asserts to ensure that a single iteration is needed.

It might be easier to review the commits one by one rather than the PR as a whole.

This simplification seems to lead to a small performance improvement (from 655s to 639s on my tests).

Any comment/suggestion/question is welcome.

@nboullis nboullis requested a review from TheoPannetier May 13, 2025 12:51
Copy link
Member

@TheoPannetier TheoPannetier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is one of the oldest ones in RangeShifter and is in dire need of refactoring, so many thanks for taking the first steps towards this. However I believe this PR would change the behaviour of the algorithm (i.e., throwing instead of re-drawing, see my comments) - but do correct me if I got it wrong.
In addition, it's quite a "sensitive" part of the simulation - both because it's central to the movement and because the syntax is difficult to follow - so I would like to have evidence that the behaviour of the function is not changed, via unit tests. I have written unit tests for the two other movement modules (kernels and random walk) but ran out of time to tackle the SMS.
In the next couple of weeks I will try to put the time in to write to assert this function keeps behaving as intended under both use and edge cases. You're of course very welcome to write some yourself (you can draw inspiration from the ones I already wrote) if you have time for it.

@TheoPannetier
Copy link
Member

Thanks for clarifying the role of the assert statements. I think this is indeed a much simpler loop than the original one. If that's ok, I still would like to wait a week or two before merging to see whether I can invest some time in writing unit tests that assert the expected and edge behaviour of the function, and ensure that the behaviour is not changed in a way we haven't anticipated.

@nboullis
Copy link
Collaborator Author

nboullis commented Jun 2, 2025

No problem to wait until you are confident I did not introduce any bug. A mistake is always possible, and you’d rather be safe than sorry. The code was rather complex, I did my best to understand it and simplify it, but I may have missed some corner cases. If you ever trigger an assertion failure with my changes, it definitely is a bug I should fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants