Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deal with x, y = None case properly #197

Merged
merged 3 commits into from
Mar 11, 2019
Merged

Deal with x, y = None case properly #197

merged 3 commits into from
Mar 11, 2019

Conversation

andrewgait
Copy link
Contributor

@andrewgait andrewgait added the bug label Jan 30, 2019
@andrewgait andrewgait added this to the 5.0.0 milestone Jan 30, 2019
@andrewgait
Copy link
Contributor Author

The tests mentioned in that issue and those in the same directory now work on spalloc, but on a spinn-3 board the second test (https://github.com/SpiNNakerManchester/sPyNNaker8/blob/master/p8_fga_tests/test_spinnaker_link_connectors/test_spinnaker_link_2pop_different_link_ids.py) doesn't fail any more when previously it was supposed to do so (and it does fail "correctly" on spalloc, I think...).

@coveralls
Copy link

coveralls commented Jan 30, 2019

Coverage Status

Coverage increased (+0.2%) to 66.203% when pulling 646e27b on retina_constraint_fix into 56b39e5 on master.

@alan-stokes
Copy link
Contributor

a placement constraint which hasnt got a placement. seems like GIGO to me. How does that work as a constraint if i can set x,y,p to none?

@andrewgait
Copy link
Contributor Author

andrewgait commented Jan 31, 2019

I agree, and perhaps the actual problem is that, when using spalloc, there's a call to the partitioner basically before almost anything else happens - so it creates vertices, and in this instance ApplicationSpinnakerLinkVertex.create_machine_vertex calls vertex.set_virtual_chip_coordinates (which then tries to add a constraint) before virtual_chip_x and virtual_chip_y have been set (they default to None). So the way round it is either to deal with the None in the constraint itself, or (possibly better) to call the algorithm that sets virtual_chip_x and virtual_chip_y before the partitioner (I'm guessing this algorithm is MallocBasedChipIdAllocator, since nowhere else calls set_virtual_chip_coordinates)...

@rowleya
Copy link
Member

rowleya commented Jan 31, 2019

Hmm, I think the fix for this might be to stop the ApplicationVertex from setting the virtual chip coordinates of the MachineVertex. The MachineVertex will receive these anyway if I remember correctly (it would need to be tested)

@andrewgait
Copy link
Contributor Author

I could have sworn that I tried changing that yesterday evening but to no effect. However it does seem to work after testing it using the tests in the issue, so this is ready to look at again.

@rowleya rowleya merged commit 7fad288 into master Mar 11, 2019
@rowleya rowleya deleted the retina_constraint_fix branch March 11, 2019 11:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants