Teaching-projects / SZE-FONYA-2021-Automata-minimization

0 stars 0 forks source link

Review #1

Closed hegyhati closed 3 years ago

hegyhati commented 3 years ago

@slimaninasreddine we will do the review process here on GitHub. If You are ready with changes, jsut write a comment and tag me.

my issues:

Please, if possible, use github properly. No file upload. commit the changes, then push them.

The input fiel should have this structure, or something very similar. The main point is, that use lists for sets, etc. States can be strings if you want to, it doesn't really matter.

{
    "states" : [0,1,2],
    "alphabet" : ["a","b"],
    "starting_state" : 0,
    "accepting_states" : [1,2],
    "transitions" : [
      {"source" : 0, "symbol": "a", "destination": 2},
      {"source" : 0, "symbol": "b", "destination": 1},
      ....
    ]
  }

or for example:

{
    "states" : ["even","odd"],
    "alphabet" : ["a","b"],
    "starting_state" : "even",
    "accepting_states" : ["odd"],
    "transitions" : {
      "odd" : {
          "a" : "even",
          "b" : "odd"
        },
        "even" : {
          "a" : "odd",
          "b" : "even"
      }
    }
  }

whichever you prefer more.

Later on you should fill the Readme.md, as we discussed.

DFA.py Seems to have a first part which fills a data structure from a json, and another, that tests if a word is accepted or not. put these into proper functions with proper arguments. Also, I don't understand, why You build the Resultat string, it seems to be rather pointless.

minimization.py code duplication, if I see correctly. After you have done the two functions I mentioned above, just import that file, and use the function there. This file also have a part which is a DFA exporter. that should not be here, that should be in the DFA.py as a function. (making a DFA class and putting these as methods would be the nicest way to do things actually, but I'm not forcing it.)

The minimization code is ~130 lines long. Break it up to digestable functions.

You seem to do some unnecessary stuff, like:

for i in range (0,len(set_of_states)):
        for j in range(0, len(set_of_states)):
            if (((set_of_states[i] in Nfs) and (set_of_states[j] ...

Why don't you do this?

for s1 in set_of_states:
        for s2 in set_of_states:
            if ((s1 in Nfs and s2 ...

would be much easier to read.

For the graphe.py: again, don't duplicate code, reuse the function from DFA.py

I can evaluate the correctness of your code, after you restructure it, now it is very difficult to read. But if you do the things I suggested above, it will be much cleaner.

slimaninasreddine commented 3 years ago

I did an update to my code, I put all the 3 codes in one "DFA.py" using functions. I hope it's okay now

hegyhati commented 3 years ago

Looking better, but still a few issues: 1) Please delete files which have outdated content with git rm. The repo should only contain the current version. 2) Why does the file reading function return data['states'],data['alphabet'],data['starting_state'],data['accepting_states'],Mat instead of just data,Mat ? It would be much cleaner. In general: make this representation cleaner. Put everything into one dict or an object, or something. PAssing around 5 different things all the time is very error-prone. 3) Don't make file the argument for any of the other functions. they should accept a representation of a DFA, whatever it is. Only the file reading function should read from a file. 4) There shouldn't be a min_file function. There should be only one function, that saves an automaton to a json, whether it is a minimized one or not. 5) The minimization is still a 140 line function. That is too much. break it down to smaller functions that have clear purpose.

So, the "skeleton" should be something like this ideally:

class DFA:

  def __init__(self, filename:str):
    pass

  def save_to_file(self, filename:str):
    pass

  def minimize(self) -> DFA:
    pass

  def plot(self,filename:str):
    pass

And all the other methods should be "hidden", that are used by minimize for example.

Of course if you don't go the OO way, things change a bit:

  def load_DFA(filename:str) -> doct:
    pass

  def save_DFA_to_file(dfa:dict, filename:str):
    pass

  def minimize_DFA(dfa:dict) -> dict:
    pass

  def plot_DFA(dfa:dict,filename:str):
    pass

Feel free to chose whichever you want, but don't pass around 5-tuples. OO is just so much simpler, as you save everything as instance variables in the __init__ function and then you can just access them with self.

But the main point is to break down minimize to MUCH SMALLER parts, like:

def __initialize_merge_table(self) -> list:
  # basically step 0 and 1. also: why do you make a whole matrix the G part of it? just don't add those fields, 
def __are_incompatible(self,idx1:int, idx2:int,current_merge_table:list) -> bool:

def __get_mergeable_states(self,current_merge_table:list) -> list:

...

Checking a 140 line function for correctness is very difficult. Checkint 20 functions each 10 lines is simple.

slimaninasreddine commented 3 years ago

I did 2 classes (class "minimizations" and the main class "DFA"), I hope it's okay

hegyhati commented 3 years ago

No, it is not, you don't really understand OO, and my comments. Please take the following as a constructive feedback, not as bullying: If you want to be a software developer, I highly suggest you to learn and do lot of coding, you have a lot to improve. And don't learn new and new modules. Learn style, architecture, clean coding, best practices, OO principles, etc. And after you did, come back here, look at your code, read my comments, and then you will understand what I wanted You to do, and how you didn't do that :-)

But, the goal of this class is not to teach you programming, but formal language theory. However, you chose to do a project, instead of tests and exams. So I have to grade the project. Code quality is bad, something around grade 3, but I would like to give you a better grade.

I'll not ask you to improve your code, as it would require a lot of learning first (for which we don't have the time now).

So instead, I ask you to make a nice "presentation" of the tool.

Make examples, show the minimized automaton generated by your program, etc.

So basically "Sell it to me".

Either make it here in md or try google collaboratory for example.

slimaninasreddine commented 3 years ago

Finally, I am very happy because I found who can tell me my weakness, I am really respect you and I am really appreciate what you do, I want to request you something, Could you please help me to improve my skills, Could you please recomende some books some channels to develop my skills, could you please accept to continue working with this code until the end of june, Because I am really want to learn from you. I don't want to continue escape with nothing. Could you please help me to improve and develop my skills. Thank you Dr Mate

hegyhati commented 3 years ago

You have the most important thing: you want to learn :-) now comes the second: you have to be patient. Not much can change in a month, it is a much slower process. Here are a few tips: 1) Learn about TDD, and try to implement for example this project that way. 2) Follow best practices: don't make functions with a lot of arguments or lines, have variable/function names that actually tell the meaning without the need for comments. 3) Do something complex. few hundreds/thousands lines of code. Make the mistakes, learn from them. Refactor the code as you progress. 4) Start another project, look back at the old one a few months later, feel the pain of not understanding what you did, refactor the code in a way, that makes it easier. 5) Play around with other languages. Don't mix functional things yet, but do something big in C for example. That will make you understand things in more depth. 6) Read good code, and try to understand it. There definitely are open source project in your preferred language. 7) There are plenty of good youtube educators, find the ones that don't just show you how to do it, but WHY you do it that way.

These are my ideas from the top of my hat.

Also, you may check this out as a starter: https://www.youtube.com/watch?v=7EmboKQH8lM

hegyhati commented 3 years ago

och, and learn about CI/CD

slimaninasreddine commented 3 years ago

Thank you Dr. Mate I will start and I will change and improve my skills

hegyhati commented 3 years ago

Go ahead, and please also don't forget about pimping this submission as well. Let me know when I can have a look again. You have time until the last exam, which is around the end of the exam period.

slimaninasreddine commented 3 years ago

I am very sorry because I need more time forlearn this skill, I wanna ask you if you can please give me grade 2 to pass this subject. Thank you Dr. Mate All the best

hegyhati commented 3 years ago

Hi! Done! But you have to accept it in neptun. All the best!