Move functions between Delta, Nfa, and mata::nfa
#329
Conversation
Adda0
force-pushed
the
move_to_delta
branch
3 times, most recently
from
1a1a054 to
8ef35fb
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## devel #329 +/- ##
==========================================
+ Coverage 72.74% 72.93% +0.18%
==========================================
Files 33 33
Lines 4146 4134 -12
Branches 953 952 -1
==========================================
- Hits 3016 3015 -1
+ Misses 755 740 -15
- Partials 375 379 +4
☔ View full report in Codecov by Sentry. |
| @@ -345,9 +346,16 @@ public: | |||
|
|
|||
There was a problem hiding this comment.
btw, defragment above needs some comment
| @@ -138,7 +133,7 @@ public: | |||
| */ | |||
| void unify_final(); | |||
|
|
|||
| bool is_state(const State &state_to_check) const { return state_to_check < size(); } | |||
| bool is_state(const State &state_to_check) const { return state_to_check < num_of_states(); } | |||
There was a problem hiding this comment.
if it is a method of delta, then the name is not good, because delta alone does not decide about whether a number is state or not. Maybe contains_state, has_state, ...
There was a problem hiding this comment.
All three options (the original is_state() in the code, contains_state() and has_state()) sound good to me. Anyone else any preferences?
There was a problem hiding this comment.
That is maybe even better. It clearly explains that the notion of is a state has no meaning for delta, and that this function instead checks whether the state is a source or a target for any transition. I would however rename it to used(State state) or used_state(State state), or, ideally, is_state_used(State state). What do you think?
There was a problem hiding this comment.
I added a method uses_state(State state) to Delta.
| * @param[in] sink_state The state into which new transitions are added. | ||
| * @return True if some new transition was added to the NFA. | ||
| */ | ||
| bool make_complete(const Alphabet& alphabet, State sink_state); |
There was a problem hiding this comment.
just complete? the make anyway looks like making a new automaton
There was a problem hiding this comment.
We can have complete() as the operation and is_complete() as the check for completeness.
There was a problem hiding this comment.
Anyone else any opinions?
| @@ -402,51 +442,6 @@ Nfa intersection(const Nfa& lhs, const Nfa& rhs, | |||
| Nfa concatenate(const Nfa& lhs, const Nfa& rhs, bool use_epsilon = false, | |||
There was a problem hiding this comment.
concatenation, this creates a new nfa, right?
There was a problem hiding this comment.
Yes. We can rename it to concatenation(). Anyone else any opinions on this?
| * @return True if the language is empty, false otherwise. | ||
| */ | ||
| bool is_lang_empty(const Nfa& aut, Run* cex = nullptr); | ||
|
|
||
| Nfa uni(const Nfa &lhs, const Nfa &rhs); |
There was a problem hiding this comment.
the annoyance this generates is really high
There was a problem hiding this comment.
nfa_union(), as for set_union() in OrdVector?
There was a problem hiding this comment.
I agree that lang_union() might be even better than nfa_union(), if we are to have some prefix to union, that is.
| @@ -509,14 +504,6 @@ Nfa determinize(const Nfa& aut, std::unordered_map<StateSet, State> *subset_map | |||
| Nfa reduce(const Nfa &aut, StateRenaming *state_renaming = nullptr, | |||
There was a problem hiding this comment.
sounds like in-place thing
There was a problem hiding this comment.
We can rename it to reduction() or reduced(). Anyone else any opinions on this?
| } | ||
| return transitions_to_state; | ||
| } | ||
|
|
||
| void Delta::add(State source, Symbol symbol, State target) { |
There was a problem hiding this comment.
do wa want insert everywhere?
There was a problem hiding this comment.
And there is a remove() below it, too. Do we want to rename to insert() and erase() everywhere?
| symbols.insert(symbol_post.symbol); | ||
| } | ||
| } | ||
| //TODO: is it necessary to return ordered vector? Would the number predicate suffice? |
There was a problem hiding this comment.
related to this TODO, maybe we want to refactor?
| @@ -167,25 +166,27 @@ cdef extern from "mata/nfa/nfa.hh" namespace "mata::nfa": | |||
| StateSet get_terminating_states() | |||
| void remove_epsilon(Symbol) except + | |||
There was a problem hiding this comment.
There was a suggestion to rename remove_epsilon() to epsilon_removal() or epsilon_removed(), or epsilon_free(). Opinions?
Adda0
force-pushed
the
move_to_delta
branch
from
8ef35fb to
a397b46
Compare
Adda0
force-pushed
the
move_to_delta
branch
from
a397b46 to
22ac0c6
Compare
Adda0
force-pushed
the
move_to_delta
branch
from
22ac0c6 to
4fe2e07
Compare
|
MacOS performance tests fail on a completely unrelated issue about missing performance test data. I will ignore the fail for now. Let us see whether it fixes itself over time. |
This PR reorganizes some functions between classes
Delta,Nfa, andmata::nfanamespace.Namely, the PR does the following:
Delta.mata::nfawhich are in-place operations or constant checks toNfa.Nfa::size()toNfa::num_of_states()to prevent further confusions about the meaning ofaut.size().