Skip to content
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

General architecture improvement #26

Open
bygu4 opened this issue Oct 31, 2024 · 1 comment
Open

General architecture improvement #26

bygu4 opened this issue Oct 31, 2024 · 1 comment

Comments

@bygu4
Copy link

bygu4 commented Oct 31, 2024

Continuing the finite_automaton redesign I think we should go on and rework the relations of the rest of the modules as well, making the architecture clearer and removing the cyclic imports. For now, I think this is the general design we could try to implement:

general_module_design

In more detail, firstly I think we should build CFG from PDA in the CFG class instead:

def to_cfg(self) -> "cfg.CFG":

Then, I think we should define the PDA intersection with the DFA explicitly, to avoid unnecessary checks, and so that Regex -> ENFA -> DFA transition chain can be used:
def intersection(self, other: Any) -> "PDA":

And also we can do the same thing in the CFG class:
def intersection(self, other: Any) -> "CFG":

On a side note, I think the CFG Variable Converter should take place in the cfg module. To rework it in a stricter way, I think we can move the object classes (i. g. finite_automaton objects, regex objects, etc.) to a separate module, and that way I think we could also avoid some potential design issues.

Moving on to indexed_grammar, I think we should implement the intersection explicitly with FST (in a way similar to CFG):

def intersection(self, other: Any) -> "IndexedGrammar":

In that case the Regex -> ENFA -> FST transition chain could be used. Then we can remove the intersection method from the FST class. I think it shouldn't be there if it returns IndexedGrammar.
Also I would suggest using CFG objects in indexed_grammar, and FA objects in fst to add stricter type annotations.

If you agree with these changes, I would like to take this issue and try to implement them.

@bygu4
Copy link
Author

bygu4 commented Nov 15, 2024

Alternatively, we can add from_cfg method to PDA. It might be better because:

  • it is probably a more natural design choice
  • it is easier to refactor
  • CFG class is already quite large.

But on other hand, it would bring inconsistency to the architecture. I think, it would have made more sense if we had regular grammar representation that finite_automaton module would depend on. Maybe that's an idea for future updates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant