SprocketLab / WS-Structured-Prediction

2 stars 1 forks source link

Not possible to create a `Ranking` object with one-indexed permutation #2

Open scottfleming opened 1 year ago

scottfleming commented 1 year ago

If I try to run the following code from the main directory:

from code.core import ws_ranking
from code.core.ws_ranking import WeakSupRanking
from code.core import ranking_utils
from code.core.ranking_utils import RankingUtils, Ranking

r_utils = RankingUtils(3)
r = Ranking(permutation = [1, 2, 3], r_utils=tmp_utils)
r_utils.ranking_to_pair_signs(r)

I get an error:

---------------------------------------------------------------------------
KeyError                                  Traceback (most recent call last)
Cell In[4], line 2
      1 r_utils = RankingUtils(3)
----> 2 r = Ranking(permutation = [1, 2, 3], r_utils=tmp_utils)
      3 r_utils.ranking_to_pair_signs(r)

File ~/Documents/GitHub/WS-Structured-Prediction/code/core/ranking_utils.py:471, in Ranking.__init__(self, permutation, r_utils, z)
    468 self.mask = np.ones(len(self.r_utils.unique_pairs))
    470 if z is None:
--> 471     self.z = self.r_utils.ranking_to_pair_signs(self)
    472 else:
    473     self.z = z

File ~/Documents/GitHub/WS-Structured-Prediction/code/core/ranking_utils.py:78, in RankingUtils.ranking_to_pair_signs(self, r)
     76             z[idx] = 1 * r.mask[idx]
     77         else:
---> 78             idx = self.pair_index_map[self.pair_key(b, a)]
     79             z[idx] = -1 * r.mask[idx]
     80 return z

KeyError: '3,1'

Two issues here: (1) It feels awkward that RankingUtils requires as part of its initialization the dimension of a Ranking object, but then the initialization of a Ranking object requires a RankingUtils object -- this feels like a circular dependency. (2) The error seems to arise because in lines 28 through 39 of ranking_utils.py (see below), self.pair_index_map is set to have zero-indexed pair keys rather than arbitrary rank values that might come from eg a Ranking object's permutation (see circular dependency issue above). So despite the fact that a and b in ranking_to_pair_signs is set to take on the arbitrary values from within the Ranking object, the self.pair_index_map object never had access to those values when it was being initialized. In practice, the way I see it, this means that Ranking objects can only ever take on zero-indexed values. This feels like a design flaw, but more importantly... (3) None of these assumptions are documented in the class descriptors or docstrings for Ranking and RankingUtils, nor are there any unit tests that I could use to make sure nothing breaks were I to try and submit a fix.

https://github.com/SprocketLab/WS-Structured-Prediction/blob/39d7da86bf043e88e9eeb62a4fa6fb926a55efa7/code/core/ranking_utils.py#L28-L39

scottfleming commented 1 year ago

Note that this also creates a problem for ties (and fractional ranks when you want to have a rank ordering like [1, 2.5, 2.5, 4])