-
Notifications
You must be signed in to change notification settings - Fork 13
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
Implement functions for segmentation #36
Conversation
Rename trans_size() and fix its implementation to return the number of transitions instead of the number of states.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no problems with pull request, it is suitable for merge. Please check it too @tfiedor .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm OK with the PR. It's well done, though I have some minor issues, that I would suggest to consider:
- Some of the names could be enhanced (also I would suggest to rename
set_initial
andset_final
toreset_initial
, since you are clearing the states, which is captured in the actionreset
, but not inset
) - On several places you use
*this = ...
, which seems to be dangerous. Maybe it is OK, but I would suggest to at least check, that this is OK to be done in C++. Since you are calling this from the method, I feel it could lead to some dangling pointers, memory corruption or some other unexpected behaviour.
Thank you for the reviews.
This is a great idea. I will apply the suggested changes.
I have seen this structure used somewhere. Nevertheless, I will do a proper research about this issue. If it reveals to be a problem (with the suggested implications), I will update the underlaying member variables instead (set of initial or final states and transition relation). |
As I said, it is just a hunch, that it could be dangerous, maybe Ondra knows more about this. So, I would maybe investigate a little about this, to avoid potential issues in future. I used to program in C++ a lot and didn't encountered this gem. I kind of like it, since it's basically a defilement of the language :-D. |
You are not wrong. I can see how this could be a bad practise at minimal, maybe even dangerous in and of itself. You have certainly piqued my interest. |
I don't have strong opinion on |
All issues with this PR are resolved and the changes we agreed upon are applied. Feel free to review the new changes (just some renamed functions, as suggested). Otherwise, from where I stand, the PR is finished. After these potential reviews, the PR is ready to be merged. |
Ok, I'm gonna merge it. |
This PR implements functions used in segmentation algorithms. Furthermore, this PR contains implementation of helper functions and generally useful member functions on class (struct)
Mata::Nfa::Nfa
.All implemented helper functions are required by segmentation algorithms, but are useful for operations on NFAs.
This PR:
OrdVector
implementationOrdVector
OrdVectors
OrdVector
implementationtrans_size()
methodNfa
Nfa
A new namespace inside
Mata::Nfa
namedMata::Nfa::SegNfa
is introduced, which includes methods applicable only on segment automata, NFAs whose state space can be split into several segments connected by ε-transitions in a chain. No other ε-transitions are allowed. As a consequence, no ε-transitions can appear in a cycle.The original method
Mata::Nfa::Nfa::trans_size()
returned a number of states instead of the number of transitions. The method was fixed to truly return the number of transitions in an automaton. Furthermore, the method was renamed toget_num_of_trans()
to follow the naming structure of an already existing methodget_num_of_states()
and not introduce a new structure for similar methods. For that reason, Python binding for the originaltrans_size()
was updated as well to call the newget_num_of_trans()
method instead.All introduced methods are documented and there are basic tests written for all of them.