sassbalint / double-cube-jump-and-stay-multilingual Goto Github PK

In commit 5dde1d7 the file Makefile.jands uses ascii2uni two times, but it is not explicitly stated in the README.md of this repository, because Makefile.jands is copied from https://github.com/sassbalint/double-cube-jump-and-stay/ repo.

Further examination shows that if you use the parameter ensure_ascii=False in impl.py you will not need any conversion of unicode characters later.

Please grant explicit rights for contributors to use, modify and redistribute the package else one must assume all rights reserved!

You can use https://choosealicense.com/ to get help.

It would be nice to place a Bibtex for the paper which are required to be cited to help future users!

In commit 5dde1d7 line 293 in impl.py the else statement explicitly sets 0:

ratio( act, max_out ) if max_out else 0, STAY ) )

but it later shows on the output as 0 > 1.7 which is obviously incorrect.

I suggest using float('inf') instead which produces the same result, but mathematically correct output generated.

In commit 5dde1d7 LANG variable in process_conll.sh#L10, process_conll.sh#L20, process_conll.sh#L10 and in multiple lines of Makefile should be replaced with something else. LANG is related to locales. Changing it results using different locales for the rest of the script. These locales may not present and result in an error. If this is intended please make it explicit. IMO changing the locale is not necessary.

In commit 5dde1d7 the file impl.py differs in tow lines from the original file

The original file line 102-104 show:

  # = if there is no NOM slot => add "NOM":None
  if "NOM" not in d:
    d["NOM"] = None

But in this repo same lines show:

  # = if there is no NOM slot => add "NOM":None
  if "nsubj" not in d:
    d["nsubj"] = None

This should be set from the CLI to be able to use the same code in both repos.

In commit 5dde1d7 sstat command should be replaced in convert2json.sh#L20 and process_conll.sh#L21 to sort | uniq -c | sort -nr else make will fail!

In commit 5dde1d7 impl.py has two strange, unreasonable variable swaps which probably due to a bug:

Line 121:

build_dc_recursively( d, fq, dvfq, dvl, de, deb )

de and deb are swaped compared to the definition of the function in line 146:

def build_dc_recursively( d, fq, vertices_f, vertices_l, edges_back, edges_fwrd ):

This order is kept for the recursion in line 84:

        build_dc_recursively( e, fq, vertices_f, vertices_l, edges_back, edges_fwrd )

But outside of the function there is two another strange swap that fix the first one:

Line 135:

  # transfer edges of the given ss into main 'cl_edges_back'
  for i in de:
    for j in de[i]:
      if i not in cl_edges_back:
        cl_edges_back[i] = {}
      cl_edges_back[i][j] = 1

Notice de which is used for cl_edges_back probably mistaken in favour of deb. In conjunction with

Line 141:

  # transfer edges of the given ss into main 'cl_edges_fwrd'
  for i in deb:
    for j in deb[i]:
      if i not in cl_edges_fwrd:
        cl_edges_fwrd[i] = {}
      cl_edges_fwrd[i][j] = 1

Notice deb which is used for cl_edges_bwrd probably mistaken in favour of de.

Correcting all these mistakes result the same output, but a more reasonable code.

BTW it is not recommended to use too short variable names in Python.

In commit 5dde1d7 the file impl.py line 279 seems a bit messed up:

      # is there a stay?
      max_out = None
      d = cl_edges_fwrd.get( act, {} ) # forward vertices
                                       # there are always one except at a ss

      if d:
        max_out = max(d.keys(), key=lambda x: (cl_vertices_f[x],x))

      if max_out and is_stay( act, max_out ):

The cl_edges_fwrd.get( act, {} ) part is strange as it allows full sentences to be pVCC-s in contrary to the comment. Is it intended? Please make it more explicit!

max() has a default argument when empty iterator is given, so the code could be simplified.

In commit 5dde1d7 line 253 in impl.py

Has a comment with a concern that the line is redundant. It truly is, as impl.py#L279 resets that variable and there is no singn of usage between the two lines.

A good IDE could spot that immediately. ;)

In commit 5dde1d7 the file impl.py there is two different ratio computation for JUMP and for STAY. It would be nice to have some reference eg. to the paper why they have to differ.

Line 176:

ratio = fq/cl_vertices_f[j]

And Line 191:

ratio = cl_vertices_f[j]/fq

I suspect the forward and backward edge differences, but without any pointer to the paper I could not find any information.

BTW the two code parts could be refactored as it has only minor differences like this.