-
Notifications
You must be signed in to change notification settings - Fork 54
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
Data.Graph.Inductive.Query.TransClos.rc is wrongly implemented #86
Comments
As observed in issue haskell#86, the implementation of the function rc in the module Data.Graph.Inductive.Query.TransClos.hs computes the graph with only the self-loops, rather than the reflexive closure of a graph. This change conserves the existing edges, although it drops their labels in the process to satisfy the desired type of the function.
Note that the function That is not a bug per se, but I feel it is counterintuitive. Mostly, the function |
@LambdaP: It is good practice to include a MWE that exposes the problem. From this MWE, you then directly get a test case. |
This was fixed in #87. |
@andreasabel I don't think you mean to, but this feels condescending. Here is a 4 years old issue about an obviously extremely wrong piece of code, for which I carefully explained why it was wrong, offered a fix, and then added a conceptual discussion warning about the ambiguities of the API. You pop in out of nowhere 4 years later to educate me about good practice, in a case where I frankly doubt that it would have been worth anyone's time to read, let alone write, an MWE for this, given how transparent the mathematical expression of the code is. Simultaneously, in the discussion about the related commit, you tell me about some entirely unrelated bug in a library, with vague instructions to check that my commit (which, again, was correcting a piece code that was hopelessly wrong and could not be relied on to do anything remotely sensible) didn't introduce bugs somewhere else, sending me down a trail to understand what's going on with code that has, in fact, nothing to do with something I did and last thought about four years ago. Please don't treat other people's time as less valuable than yours. |
Sorry, @LambdaP, didn't mean to be condescending, apologies if you got this wrong. Yet I stand to my principles. The most helpful bug reports include a MWE. Any bug fix should include a regression test. I won't waiver on this. |
In the module
Data.Graph.Inductive.Query.TransClos
, the functionrc
has the following description:Finds the reflexive closure of a directed graph.
Given a graph G=(V,E), its transitive closure is the graph:
G* = (V,Er union E) where Er = {(i,i): i in V}
However, its implementation of the function rc is:
This very obviously computes the graph (V, Er), using the previous notation.
Proposed solution: replace the code with:
I am following this issue with the corresponding PR.
The text was updated successfully, but these errors were encountered: