Implement missing functions #40
Conversation
|
After short discussion, we decided to create new files |
There was a problem hiding this comment.
I'm OK with this PR, it's well done.
I'm just wondering, what is the point of rewriting intersection? It seems to be much more complicated and there are lots of tripple nested loops, that could potentially lead to some huge computation time (maybe I'm wrong).
Also I would advise against having PR that includes changes of other PR :).
The code implementing the normal intersection without preserving epsilon transitions is kept the same as the previous implementation of the intersection from Vata2. There were already some TODOs suggesting rewriting the implementation of intersection. The previous long function was only split into multiple smaller ones to reuse some parts of the code common to both non-preserving and preserving intersections. I have used the same code for epsilon preserving intersection, only updated behaviour when epsilon symbol is encountered. I can understand your concerns regarding computation time requirements. This could use some refactorization. I will inspect the options further and see if I can come up with something suitable. Nevertheless, this could be a good starting point in rewriting the previous intersection implementation, as recommended by the previous TODOs.
After discussing this with @martinhruska, we agreed on creating this and #41 PRs to allow quicker reviews. The code would not compile without commits from #36. When #36 is merged, this PR will be rebased on the new |
I assumed, this was the case, so I'm just "warning" against this practice to be "standard" in future :-). |
|
Dully noted. The alternative solution would be to create these PRs on my fork of Mata and point you there for the reviews. For the possible future uses (I really hope that will not be necessary), would you as a reviewer prefer the redirection to other repository to this approach? Or, do you have a better idea how to solve this? I could not find a cleaner solution. I could create a new branch directly on VeriFIT/Mata for segmentation, another for this PR and yet one more for noodlification. Then I could create a new PR for each new branch, do the code reviews, close the PRs and create these PRs, already rebased on the updated |
|
We need to fix the process to not let you wait for 10 days on our reviews. Then this hacking with PRs would not be necessary. |
Warn user that the equivalence check function without specified alphabet passed to the function may be inefficient.
Instead of constructors of intersection, call Intersection::compute to create a new instance of Intersection class with intersection computed.
|
All issues with this PR are resolved and the changes we agreed upon are applied. Please, feel free to review the new changes (commits 6cf1054 and further). I have moved concatenation and intersection implementation to their own Otherwise, from where I stand, the PR is finished. After these potential reviews, the PR is ready to be merged. |
Once #36 is merged, this PR will be rebased on the new
develbranch and opened for merging. For now, the changes introduced in the new commits since #36 are ready for review.This PR implements automata operations requested in #25 except for simulation-based minimization (which will be implemented later by @jurajsic).
The implemented operations are:
Some additional helper functions were introduced, too.
The code has been tested with some simple tests. Nevertheless, it would be beneficial to add more tests for the introduced functions.
In the case of intersection, the previous implementation was reused and updated to give the option to choose between classic and epsilon transitions preserving intersection.