search

dice-group / AGDISTIS

AGDISTIS - Agnostic Named Entity Disambiguation
http://aksw.org/Projects/AGDISTIS.html
GNU Affero General Public License v3.0
141 stars 37 forks source link

In BFS the findNode is not used (nodes always added even if already exists) #71

Closed khaledadel19 closed 5 years ago

khaledadel19 commented 5 years ago

Line 49 in BreadthFirstSearch checks for the existence of targetNode in findNode. However the key used in findNode is targetNode.getObject(). Same for line 50 in findeNode.get().

RicardoUsbeck commented 5 years ago

Thanks for pointing out that issue. It is indeed good, that this if never worked from the beginning six years ago!

It is a bug!

We should remove private static HashMap<String, Node> findNode = new HashMap<String, Node>(); completely from the class as this static hashmap screws up things when using two graphs.

Either you do a pull request with the removed findNode field or I will do that in April, when I am back from vacations. @DiegoMoussallem what do you think?

DiegoMoussallem commented 5 years ago

The nodes are always added indeed, however, it does not affect AGDISTIS' quality performance because of the hashmap used in edu.uci.ics.jung.graph.DirectedSparseGraph; replaces and not duplicates when the same node is added to the graph. We can do what you suggested @RicardoUsbeck because this "static" will screw up certainly when two graphs are in the game, but we can also fix it and check if we have some gain in terms of time-performance. We could try both options.

khaledadel19 commented 5 years ago

Thanks for your swift responses, I actually tried to activate this part. I noted that activating this check using static, causes issues even when working with only one graph, changes on the weights from one request (during HITS) affects the consequent requests if the same node is found.

RicardoUsbeck commented 5 years ago

Yes, that is why I would remove this HashMap and just keep the else part

vdanielupb commented 5 years ago

should be fixed now.

RicardoUsbeck commented 5 years ago

Can someone, maybe @DiegoMoussallem make sure we get the same results as in the latest MAG paper?

DiegoMoussallem commented 5 years ago

Sure, I am going to check it and let you know. ;)

DiegoMoussallem commented 5 years ago

Checked, the same results are obtained with this commit.