sinemgetir / state-elimination-mt

created for TCC 2017
1 stars 3 forks source link

Review: NMF #5

Closed MSharbaf closed 4 years ago

MSharbaf commented 7 years ago

It is an interesting solution for the state elimination case study. However, the SHARE demo is not provided and I was not able to run the NMF solution with Visual Studio, due to existence of some reference error in the files.

In addition, evaluation result is not provided for the solution and therefore I could not verify the correctness of the regular expressions that are generated as final results. However, it is assumed that the correctness is provided for all the models.

Finally the paper is well written and well structured.

The evaluation sheet provides more details for the evaluation of this solution. NMF-ttc-evaluation-sheet-state-elimination.xlsx

georghinkel commented 7 years ago

Sorry for the reference error, but actually, you should be able to run the solution without compiling in Visual Studio because the binaries are checked in. Anyway, I will check the reference errors.

dstrueber commented 7 years ago

Review #2:

Following some communication with the case author, I was able to run the solution and reproduce the excellent execution times as indicated in the paper. I obtained correct solutions rapidly for all models except for leader6_5, leader6_6 and leader6_8.

Despite being based on in general-purpose-code, I think the solution is largely adequate for the problem. The involved graph structures are relatively simple, and so it's fair to expect that developers can understand the transformation by looking at the code. I mostly found the listings readable, except for some NMF-specific terms, like "FirstOrDefault". These should be better explained in the text.

Points for improvement: