-
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
Nfa compose #389
Nfa compose #389
Conversation
…insert_word methods
…en inserting a word.
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.
The PR looks good to me in general. The plan now is to merge this PR immediately, so I can apply the changes here on string solving. Resolve the review issues later on, when there is time.
Furthermore, the things that should be modifed are:
- nfa::Nfa::insert_word() can have optional target word
- nft::Nft::insert_word() can have optional target word
- nft::Nft::insert_word_by_parts() can have optional target word
- nft::Nft::insert_identity() has an overload with a vector of symbols, creating an identity for all the symbols within.
* @param word The nonempty word to be inserted into the NFA. | ||
* @param tgt The target state where the word ends. It must already be a part of the automaton. | ||
*/ | ||
void insert_word(const State src, const Word &word, const State tgt); |
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.
Future TODO: The target state should be optional and be returned from the function afterwards.
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.
So, when the target state will not be specified, the method will create a sequence of states from src
(already existing) to tgt
(newly created), where tgt
does not have any successors and is not a final state. It that correct?
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.
Yes. And the newly created target will be returned from the function so that the user knows where to continue.
* @param[in] lhs_first_aux_state The first auxiliary state in @p lhs. Two auxiliary states can not form a product state. | ||
* @param[in] rhs_first_aux_state The first auxiliary state in @p rhs. Two auxiliary states con not form a product state. | ||
* @return NFT as a product of NFTs @p lhs and @p rhs with ε handled as regular symbols. |
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 would explain here what are these auxiliary states for.
*/ | ||
Nft product(const Nft& lhs, const Nft& rhs, const std::function<bool(State,State)> && final_condition, | ||
const Symbol first_epsilon = EPSILON, std::unordered_map<std::pair<State,State>, State> *prod_map = nullptr); | ||
std::unordered_map<std::pair<State,State>, State> *prod_map = nullptr, | ||
const State lhs_first_aux_state = Limits::max_state, const State rhs_first_aux_state = Limits::max_state); |
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.
Are the states used anywhere in Delta
? Because if so, the Limits::max_state
of memory is being allocated.
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.
No, these states are not used in Delta if they are not already a part of it. Essentially, it prevents two states from Delta, which are greater than the specified auxiliary state (implying that these states are auxiliary), from forming a product state.
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.
You are right. I kind of saw it in the code later on, but I wanted to make sure.
|
||
#include <fstream> |
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.
Unnecessary import, probably left behind from debugging.
This pull request includes:
nft::project_out()
.Nft::insert_word()
,Nft::insert_word_by_parts()
, andNft::insert_identity()
for inserting a given word (as one word or a vector of its parts, each for a different level) between source and target states (source and target states are equal for the identity).nft::insert_level()
andnft::insert_levels()
for inserting levels with adefault_symbol
on transitions with newly inserted levels. These functions are utilized in matching levels of synchronization between two transducers.nft::intersection()
andnft::algorithm::product()
to handle transducers withEPSILON
andDONT_CARE
transitions and a set of auxiliary states (two auxiliary states cannot create a product state).nft::compose
for computing the composition of two transducers.