ELITR / SLTev

SLTev is a tool for comprehensive evaluation of (simultaneous) spoken language translation.
8 stars 3 forks source link

Be more pythonic #9

Open obo opened 3 years ago

obo commented 3 years ago

SLTev is not written in a nice python way. Aside from worse readability, it brings needless platform dependence. For instance making os.system calls like this one: https://github.com/ELITR/SLTev/blob/ddc77fff7101d58a8b5d0d8286658480dc518330/SLTev-scripts/SLTev#L326 is:

Make SLTev a module which can be either imported or directly called (it should contain the standard test at the end and call its "main" if it is run directly).

If people only import SLTev, they should have access to the evaluation routines, to be directly called from within the python. Avoid external tools as much as possible! (with the exception of mwerSegmenter or GIZA).

pyRis commented 3 years ago

A more suitable way will be to use subprocess library. In general, subprocess is almost always preferred over os.system() calls.

obo commented 3 years ago

The point here is not to achieve any parallelization (for which subprocess would be the solution). The way os.system() is used in current SLTev is just to make a function call. :-//

obo commented 3 years ago

Rishu, could you please test the current code and close this issue if SLTev can be now used both:

obo commented 3 years ago

Matusi, this is also something you might immediately see and close. If SLTev is easy to use both as a script and as a module directly from python, mark this issue as closed. Thanks, O.

mzilinec commented 3 years ago

@obo I think that it is usable as both script and a module, but still, the title "Be more pythonic" - is not the case yet. There are still things such as calling python evaluator.py with os.system() - this is really not good.

obo commented 3 years ago

@mzilinec I fully agree. Please rename the issue (or fix the call if it is very easy -- which I do not really expect).