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

Fixing issue #21 #22

Merged
merged 5 commits into from
Mar 15, 2018
Merged

Fixing issue #21 #22

merged 5 commits into from
Mar 15, 2018

Conversation

mmuesly
Copy link
Contributor

@mmuesly mmuesly commented Mar 12, 2018

I found an error in the strong component tracing while debugging another project.

So, I suggest the following fix and added a few more examples, ensuring that the fix is working.

…and extended the test case set for this algorithm.
@mmuesly
Copy link
Contributor Author

mmuesly commented Mar 12, 2018

It seems like travis is checking the source code using pmd, but the codebase is not completely in shape for pmd checks. I fixed the util subproject as this is the area where my changes reside, but it seems like I would have to fix all automata lib modules to make pmd pass in the travis ci build.
What do you recommend for solving the build errors?

@mtf90
Copy link
Member

mtf90 commented Mar 12, 2018

It seems, you directly invoked the PMD maven-plugin from the CLI (which uses some default configuration). The Automatalib build process uses a specific configuration (and exclusions) file. To replicate what Travis does, you can invoke mvn clean install -Pcode-analysis (+ ,bundles for javadoc) in the util module.

From what I saw, after reverting 6cba2a5, it was mostly just formatting stuff. If you don't mind, you can grant me push access and I can look into this.

Also, I have to think about your changes to the SCCListener interface. Previously, when finding an SCC, you would receive all nodes at once and immediately know how many nodes (Collection.size()) a component contains. In your version, you would (at the end) receive for every node its component id (=lowLink?) but can't tell, when a component is full/finished until the n-th invocation. I think, the first version may be more convenient for users who want to use a custom listener (cf. SCCs#findSCCs). I'll probably look into the algorithm myself for this. But you already did the majority of the work by writing test cases and providing a working implementation, so thank you for that already 👍.

@mmuesly
Copy link
Contributor Author

mmuesly commented Mar 13, 2018

I granted you push access. Feel free, to adapt the PR.

You are right about the SCCListener change. TheAlgorithm in the version as proposed in Tarjan's paper, needs a fine grained control about backtracking during DFS. Up to my understanding this is currently not supported sufficient well by the graph traversal infrastructure to maintain the old SCCListener interface. But I am open for discussions.
Thanks for mentioning the -Pcode-analysis flag.

make checkstyle/pmd happy
- revert most of the changes of 8231a67 because they introduced a change in
  the API and unnecessarily traversed nodes multiple times. However the commit
  discovered one core issue, which was during cycle detection the update of
  the SCC id with the start number (wrong) rather than the scc id (right).

  This fix, along with correct handling of cached values is now implemented
  ontop of the old code-base.
- Merged the "AdvancedSCCTest" with the regular one.
- Refactored internal variable names to be more precise and added JavaDoc/
  comments on what (some of) the individual variables/methods actually do.
@mtf90 mtf90 merged commit ceb9f96 into LearnLib:develop Mar 15, 2018
@mmuesly
Copy link
Contributor Author

mmuesly commented Mar 18, 2018

Thank you for locking into this and fixing the algorithm!

mtf90 pushed a commit that referenced this pull request Apr 13, 2018
* Fixing an error in the Tarjan's strong connected component algorithm and extended the test case set for this algorithm.

* Making pmd pass in the util subproject

* Revert "Making pmd pass in the util subproject"

This reverts commit 6cba2a5.

* reformatting

make checkstyle/pmd happy

* Rework fix for SCC algorithm

- revert most of the changes of 8231a67 because they introduced a change in
  the API and unnecessarily traversed nodes multiple times. However the commit
  discovered one core issue, which was during cycle detection the update of
  the SCC id with the start number (wrong) rather than the scc id (right).

  This fix, along with correct handling of cached values is now implemented
  ontop of the old code-base.
- Merged the "AdvancedSCCTest" with the regular one.
- Refactored internal variable names to be more precise and added JavaDoc/
  comments on what (some of) the individual variables/methods actually do.

(cherry picked from commit ceb9f96)
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.

None yet

2 participants