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

Possible bug in poutine/trace_messenger.py, function identify_dense_edges #3245

Closed
xrival opened this issue Jul 21, 2023 · 4 comments
Closed
Labels
Milestone

Comments

@xrival
Copy link

xrival commented Jul 21, 2023

Hi,

I found some strange code pattern in function identify_dense_edges, in file poutine/trace_messenger.py.
I notice two sequential instances of:
if site_is_subsample(node): continue
The first one appears at line 17 (in the main loop of the function) and the second one at line 21 (in the nested loop).
It seems the second condition will never be reached; if it is, the first one would be selected, going to the next iteration of the main loop.

Should the second not be as follows ?
if site_is_subsample(past_node): continue

(this would be consistent with considering only pairs of non subsample nodes).

Thanks,

Regards,

Xavier

@fritzo fritzo added the bug label Jul 22, 2023
@fritzo
Copy link
Member

fritzo commented Jul 22, 2023

Hi @xrival, indeed that line looks like a bug. Feel free to submit a one-line fix PR, or wait until we get around to fixing it ourselves. Many eyes 👀

@xrival
Copy link
Author

xrival commented Jul 23, 2023

Hi @fritzo,
Many thanks! I will try to do so in the next few days.

@fritzo
Copy link
Member

fritzo commented Jul 24, 2023

Hi @xrival mind if I quick fix this to make it into our 1.8.6 release? Thanks again for reporting it.

@fritzo fritzo added this to the 1.8.6 milestone Jul 24, 2023
@xrival
Copy link
Author

xrival commented Jul 26, 2023

Hi @xrival mind if I quick fix this to make it into our 1.8.6 release? Thanks again for reporting it.

Sure, no problem! I am traveling so I could not do it quick. Thanks for the quick replies and fix!

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

No branches or pull requests

2 participants