Conversation
|
Hey. Thank you for the PR. It looks great. I will do a proper review when I have some time. |
|
@ondrik Could you have a quick look to confirm that the PR contains what you wanted? |
Added new file learning.cc for L*, NL* Added example program - use of learning edited CMakeLists accordingly Fixed a bug with rfsa_closure -- property violated when it shouldn't because of prime rows evaluating of prime rows was fixed
There was a problem hiding this comment.
Overall, it looks great. Thanks for the PR. As of now, I am not reviewing the feasibility of the implemented algorithm. I will leave that up to @ondrik. Other than that, here are some initial thoughts about the formal requirements on the introduced changes.
| using namespace mata::nfa; | ||
| using namespace mata; | ||
| using namespace std; | ||
| using namespace mata::utils; | ||
| using namespace mata::nfa::algorithms; |
There was a problem hiding this comment.
Never use using directives in the header file. It poisons the namespace of any file where this header file is included.
| #include <vector> | ||
| using namespace mata::nfa; | ||
| using namespace mata; | ||
| using namespace std; |
There was a problem hiding this comment.
Global using namespace std; directive is generally considered a bad practice. Some projects allow this directive inside a function definition or a class definition, but in Mata, we never use this directive with std. It would be great if you could remove it and use the scope resolution operators std::.
There was a problem hiding this comment.
For now, this location is fine. We will move applications of Mata such as this into a separate subfolder in the future. Something like mata/app/learning/ and similar.
| ParameterMap params; | ||
| params["algorithm"] = "nlstar"; | ||
| Nfa res1 = learn(aut, params); | ||
| res1.print_to_dot(std::cout); | ||
|
|
||
| params["algorithm"] = "lstar"; | ||
| Nfa res2 = learn(aut, params); | ||
| res2.print_to_dot(std::cout); |
There was a problem hiding this comment.
Could you add a comment explaining what is happening in this example?
There was a problem hiding this comment.
All structures and classes should adhere to the UpperCamelCase naming convention.
| * @return true if the word is accepted by teacher | ||
| * @return false if its not accepted | ||
| */ | ||
| bool membership_query(Nfa teacher, Word word); |
There was a problem hiding this comment.
Boolean check should start with is_ prefix, or contains_, is_member(), or similar.
| * @brief Constructs a conjecture automaton | ||
| * first defines all the states of the conjecture, |
There was a problem hiding this comment.
There needs to be an empty line after @brief to start a long description. Or you must specify the beginning of a new block using @....
| */ | ||
| bool equivalence_query(Nfa teacher, Nfa conjecture, Word& cex); | ||
|
|
||
| Nfa learn(Nfa teacher, const ParameterMap& params); |
There was a problem hiding this comment.
learn_nfa_from_... or similar? I would try to use a more descriptive name.
| Nfa conjecture; | ||
| OT table(alphabet, teacher, params); |
There was a problem hiding this comment.
Why are there different spacings?
| S.clear(); | ||
| Splus.clear(); | ||
| all.clear(); | ||
| E.clear(); |
There was a problem hiding this comment.
The names of all of these structs should be more descriptive.
There was a problem hiding this comment.
The entire contents of this header file should be inside a new namespace for learning the automata. Something like mata::nfa::learning or just mata::learning (if multiple automata are supposed to be learned in the future, not just (N/D)FAs), or use even more specific name in place of learning which is quite a lot general and does not convey well what is the namespace supposed to encapsulate.
2 participants
This PR includes implementation of L* and NL* automata learning algorithms for inferring DFA or RFSA from an unknown set.
https://people.eecs.berkeley.edu/~dawnsong/teaching/s10/papers/angluin87.pdf
https://www.ijcai.org/Proceedings/09/Papers/170.pdf
Note: I wasn't sure where this should belong in the library so I created a new file.