Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should block ability to stumble into infinite while loop in percentile calculation for dff #1262

Closed
EricThomson opened this issue Feb 1, 2024 · 4 comments

Comments

@EricThomson
Copy link
Contributor

When calculating dff there is a percentile calculation that has a try/except block in a while loop. If the exception is triggered every run, the while loop never breaks. 😱 I ran into this today. We should fix this design flaw

while err:

@EricThomson
Copy link
Contributor Author

EricThomson commented Feb 2, 2024

Yikes this is a consequence of functions being deprecated in scipy. Will temporarily lock to < 1.12 for now and am adding to roadmap to fix. (Actually I found a better fix for now, but this is going to be a headache: adding to roadmap to just uncork warning and handle all deprecation warnings).

@kushalkolar
Copy link
Collaborator

kushalkolar commented Feb 2, 2024

Oh my, I cannot comprehend the amount of 🍝 in that function 😂 . This is also an example of why bare except is generally a bad idea. From a first glance this could be remedied by adding a finally block and putting a counter inside the except and use the finally block to force exiting the loop if more than n exceptions are raised. But given how deeply nested that function is, I think it can just be designed better.

@EricThomson
Copy link
Contributor Author

EricThomson commented Feb 2, 2024

Kushal -- I started with a counter, but I ended up unhappy with this approach as it leaves the smelly code in there. 😄 As you said, it can be designed better. The problem in this case is that it depends (after you go down the stack enough) on a deprecated function, so it will always hit the counter limit. I need to do a comprehensive sweep of Caiman for soon-to-be deprecated functions! There are lots of them.

@EricThomson
Copy link
Contributor Author

Fixed with #1288

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

No branches or pull requests

2 participants