_get_tags_interface fails to execute

[sergiodavies]

When asking the iptags, the GetTagsInterface class terminates due to a clash in the naming of variables in the run function.

[alan-stokes]

Hi Sergio,

your change makes a misnaming of what tags are. Youve now named tags into iptags, but then you add reverseiptags into this list. Yet the naming convetion of the entire set of iptags and reverse iptags are tags, not iptags as youve now done.

Id recommend reverting this change and trying to fix it whilst keeping the name tag.

Whats exactly the issue? as I cant see a name class from the orginal, as all the changes are just a refactor, and theres self._tags and tags which dont overlap.

[sergiodavies]

HI Alan,

Sorry, but the modifications you made in commit c6444a917565ec85690c17a233372b2a95c2b7bc on the file spinnman/_threads/_get_tags_interface.py: https://github.com/SpiNNakerManchester/SpiNNMan/commit/c6444a917565ec85690c17a233372b2a95c2b7bc

have rendered the class unusable due to a variable name clash (did you test the class after the changes before committing?). I had to revert your commit to fix the problem. If you think that the name "iptags" is inappropriate, let's come up with a different name for that variable.

Regards

Sergio

Dr Sergio Davies

Research associate - APT Group Room IT 302 - IT Building School of Computer Science Oxford Road, Manchester The University of Manchester M13 9PL, U.K.

mailto: sergio.davies@manchester.ac.uk mailto: sergio.davies@gmail.com http://apt.cs.man.ac.uk/people/daviess/

2015-04-09 9:27 GMT+01:00 Alan Stokes notifications@github.com:

Hi Sergio,

your change makes a misnaming of what tags are. Youve now named tags into iptags, but then you add reverseiptags into this list. Yet the naming convetion of the entire set of iptags and reverse iptags are tags, not iptags as youve now done.

Id recommend reverting this change and trying to fix it whilst keeping the name tag.

Whats exactly the issue? as I cant see a name class from the orginal, as all the changes are just a refactor, and theres self._tags and tags which dont overlap.

— Reply to this email directly or view it on GitHub https://github.com/SpiNNakerManchester/SpiNNMan/issues/26#issuecomment-91148765 .

[alan-stokes]

Hi Sergio,

So those links werent very helpful to me alas. But what i was able to glean was your talking about somethign that happened on march the 3rd. This means the code has been tested quite a lot since then, as I'm persuming that these were changes from the tag allocator branch which came into the master around the beginning of march, and theres been the front end commong stuff, and your buffered in branches merged in and tested since then.

Rowley and myself had tested these bits thoughly before releaseing the complete pre-release, (we used the spike io test in pynn examples and that ran with no issue, and using both a verse and iptag).

In what way is it unusable? give error emssages. looking from the file, i cant see what this naming issue is, and wasnt aware of a issue till now. Thus youll need to give more info than just "its unusable"

[sergiodavies]

HI Alan,

If you look at the method, in the link I sent before ( https://github.com/SpiNNakerManchester/SpiNNMan/commit/c6444a917565ec85690c17a233372b2a95c2b7bc ): def run(self): - line 40 of the file _get_tags_interface.py in line 52 there is the definition of variable "tags": tags = dict()

and then populates the dictionary between line 53 and 59. After your modifications, line 61 declares again variable "tags" as list:

tags = list()

(before the modifications this variable was named "iptags").

Since this last variable has a name clash with the one defined in line 52 (see above), all the values which were stored in the dictionary are deleted before being used, and with your modifications lines 62 to 79 were using the same list as source (line 64) and to append (lines 71 and 77). Therefore the class was unusable, and I had to revert these modifications.

Hope this clarifies the issue.

Cheers

Sergio

Dr Sergio Davies

Research associate - APT Group Room IT 302 - IT Building School of Computer Science Oxford Road, Manchester The University of Manchester M13 9PL, U.K.

mailto: sergio.davies@manchester.ac.uk mailto: sergio.davies@gmail.com http://apt.cs.man.ac.uk/people/daviess/

2015-04-09 10:04 GMT+01:00 Alan Stokes notifications@github.com:

Hi Sergio,

So those links werent very helpful to me alas. But what i was able to glean was your talking about somethign that happened on march the 3rd. This means the code has been tested quite a lot since then, as I'm persuming that these were changes from the tag allocator branch which came into the master around the beginning of march, and theres been the front end commong stuff, and your buffered in branches merged in and tested since then.

Rowley and myself had tested these bits thoughly before releaseing the complete pre-release, (we used the spike io test in pynn examples and that ran with no issue, and using both a verse and iptag).

In what way is it unusable? give error emssages. looking from the file, i cant see what this naming issue is, and wasnt aware of a issue till now. Thus youll need to give more info than just "its unusable"

— Reply to this email directly or view it on GitHub https://github.com/SpiNNakerManchester/SpiNNMan/issues/26#issuecomment-91162637 .

[alan-stokes]

So what your seeing is a block of code that creates a collection of threads to call spinnamn for what tags are already on the machine (52 to 60), and then the response of that is then being added to the list of tags (yes i now see the issue [its likely we never search the machine for its tags initially, thus why its not been pciked up till now]) but your change was acutally the wrong side of the coin,

you should have changed the dictonary to be instead of tags to be tag_request_threads or something simular, it being used to track which thread is asking for which tag index in the tag table on machine (we really shoudl call it something else than iptag tag table as it no longer is that).

thatll remove your nameing conflict and be explicit. regards

[sergiodavies]

Hi Alan,

The class now works! Obviously, feel free to change the name as you prefer (to something which does not clash).

Regards

Dr Sergio Davies

Research associate - APT Group Room IT 302 - IT Building School of Computer Science Oxford Road, Manchester The University of Manchester M13 9PL, U.K.

mailto: sergio.davies@manchester.ac.uk mailto: sergio.davies@gmail.com http://apt.cs.man.ac.uk/people/daviess/

2015-04-09 10:47 GMT+01:00 Alan Stokes notifications@github.com:

So what your seeing is a block of code that creates a collection of threads to call spinnamn for what tags are already on the machine (52 to 60), and then the response of that is then being added to the list of tags (yes i now see the issue [its likely we never search the machine for its tags initially, thus why its not been pciked up till now]) but your change was acutally the wrong side of the coin,

you should have changed the dictonary to be instead of tags to be tag_request_threads or something simular, it being used to track which thread is asking for which tag index in the tag table on machine (we really shoudl call it something else than iptag tag table as it no longer is that).

thatll remove your nameing conflict and be explicit. regards

— Reply to this email directly or view it on GitHub https://github.com/SpiNNakerManchester/SpiNNMan/issues/26#issuecomment-91176927 .

[alan-stokes]

Well im not going to change the file whilst your working on that section old boy. Ill leave it to you to change, as what you've left it as is unclear and just because it works is not good enough reason to leave it. Thus im keeping this issue open.

[sergiodavies]

Hi Alan,

I've committed the changes to the class, which is now working, and I've moved on to fix something else. If you think the changes are inappropriate, obviously, feel free to do the modifications as you envision they are suitable.

Regards

Dr Sergio Davies

Research associate - APT Group Room IT 302 - IT Building School of Computer Science Oxford Road, Manchester The University of Manchester M13 9PL, U.K.

mailto: sergio.davies@manchester.ac.uk mailto: sergio.davies@gmail.com http://apt.cs.man.ac.uk/people/daviess/

2015-04-09 12:39 GMT+01:00 Alan Stokes notifications@github.com:

Well im not going to change the file whilst your working on that section old boy. Ill leave it to you to change, as what you've left it as is unclear and just because it works is not good enough reason to leave it. Thus im keeping this issue open.

— Reply to this email directly or view it on GitHub https://github.com/SpiNNakerManchester/SpiNNMan/issues/26#issuecomment-91204112 .

[alan-stokes]

Sergio, the idea of team programming is that we debate and come to a agreement that we both agree with. Im not going to just override your changes just becuse I disagree with them. We're meant to discuss and then whoever is most suited/available to do so makes said changes.

Why do you feel my proposed changes are inappropriate?