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.
|
self.items = list(range(d)) |
|
self.unique_pairs = [] |
|
for i in range(d): |
|
for j in range(i + 1, d): |
|
self.unique_pairs.append((i, j)) |
|
|
|
self.pair_index_map = {} |
|
k = 0 |
|
for i in range(d): |
|
for j in range(i + 1, d): |
|
self.pair_index_map[self.pair_key(self.items[i], self.items[j])] = k |
|
k += 1 |
If I try to run the following code from the main directory:
I get an error:
Two issues here:
(1) It feels awkward that
RankingUtilsrequires as part of its initialization the dimension of a Ranking object, but then the initialization of aRankingobject requires aRankingUtilsobject -- 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_mapis set to have zero-indexed pair keys rather than arbitrary rank values that might come from eg a Ranking object'spermutation(see circular dependency issue above). So despite the fact thataandbinranking_to_pair_signsis set to take on the arbitrary values from within theRankingobject, theself.pair_index_mapobject never had access to those values when it was being initialized. In practice, the way I see it, this means thatRankingobjects 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
RankingandRankingUtils, nor are there any unit tests that I could use to make sure nothing breaks were I to try and submit a fix.WS-Structured-Prediction/code/core/ranking_utils.py
Lines 28 to 39 in 39d7da8