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

Insert entries above blocks of similar entries #222

Merged
merged 3 commits into from
Mar 2, 2016
Merged

Insert entries above blocks of similar entries #222

merged 3 commits into from
Mar 2, 2016

Conversation

mundya
Copy link
Member

@mundya mundya commented Feb 16, 2016

Insert entries above blocks of entries of equivalent generality. This
makes that the Python implementation of OC matches (a) the C version for
SpiNNaker (b) that described in the HPSR submission.

For table ordering, see
https://github.com/project-rig/rig_routing_tables/blob/master/include/ordered_covering.h#L16-L57

For downcheck preferring to modify more significant bits in merges see
https://github.com/project-rig/rig_routing_tables/blob/master/include/ordered_covering.h#L285-L292

For merge application, see
https://github.com/project-rig/rig_routing_tables/blob/master/include/ordered_covering.h#L409-L485

Insert entries above blocks of entries of equivalent generality. This
makes that the Python implementation of OC matches (a) the C version for
SpiNNaker (b) that described in the HPSR submission.

For table ordering, see
https://github.com/project-rig/rig_routing_tables/blob/master/include/ordered_covering.h#L16-L57

For downcheck preferring to modify more significant bits in merges see
https://github.com/project-rig/rig_routing_tables/blob/master/include/ordered_covering.h#L285-L292

For merge application, see
https://github.com/project-rig/rig_routing_tables/blob/master/include/ordered_covering.h#L409-L485
@mundya
Copy link
Member Author

mundya commented Feb 16, 2016

@mossblaser - the coverage seems to indicate a problem with stuff touched by #217 rather than by this PR.

return routing_entries, aliases
# Iterate through the old table copying entries acrosss
insert = 0
for remove, entry in enumerate(self.routing_table):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I would prefer "remove" to be "i" since "remove" does not say "index in the (old) table" to me.

@mossblaser
Copy link
Member

Superficial criticisms aside this looks good to me.

The coverage issue (as mentioned offline) appears to be a result of coveralls doing something a bit strange and I'll investigate at some other time. As such I'm happy to ignore it for this PR since we've both verified the coverage locally is actually still 100%.

As suggested by @mossblaser

I've retained the brackets in `not (key & bit)` because I feel it's
clearer than `not key & bit`.
@mossblaser
Copy link
Member

I've retained the brackets in not (key & bit) because I feel it's
clearer than not key & bit.

I would agree :).

LGTM! Merge at will once travis is happy.

... I must figure out what's going on with coveralls at some point :/

mundya added a commit that referenced this pull request Mar 2, 2016
OrderedCovering: Insert entries above blocks of similar entries
@mundya mundya merged commit 3b3eea9 into master Mar 2, 2016
@mundya mundya deleted the update-oc branch March 2, 2016 09:13
@mundya
Copy link
Member Author

mundya commented Mar 2, 2016

@rowleya / @alan-stokes - this should improve the compression of the ucolumn tables.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants