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

Remove 18 #248

Closed
wants to merge 13 commits into from
Closed

Remove 18 #248

wants to merge 13 commits into from

Conversation

Christian-B
Copy link
Member

@Christian-B Christian-B commented Mar 14, 2024

These PRs remove some of the hard coded 18 Cores

MulticastRoutingEntry, FixedRouteEntry and MulticastRoutingTableByPartitionEntry now have a shared BaseMulticastRoutingEntry.

  • spinnaker_router and back calculation in just a single place.
  • Utility methods removed from Router
  • allow creating one form another using just spinnker_route
  • safety code so link id and processor id removed for speed. But can now be added in one place

MAX_LINKS_PER_ROUTER = 6 moved to constants

Must be done at the same time as:
SpiNNakerManchester/SpiNNMan#395
SpiNNakerManchester/PACMAN#550
SpiNNakerManchester/SpiNNFrontEndCommon#1169

tested by:
SpiNNakerManchester/IntegrationTests#260

@coveralls
Copy link

coveralls commented Mar 18, 2024

Coverage Status

coverage: 93.762% (-0.005%) from 93.767%
when pulling ba2d7eb on remove_18
into 2c5bea7 on master.

Comment on lines +36 to +39
.. note::
The processor_ids and link_ids parameters are only optional if a
spinnaker_route is provided. If a spinnaker_route is provided
the processor_ids and link_ids parameters are ignored.
Copy link
Member

Choose a reason for hiding this comment

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

A potential way to avoid this "multi-constructor" is instead to have a static method which is something like "def from_procs_and_links(processor_ids, link_ids)" which does the conversion and just calls the spinnaker_route constructor. That then makes it harder to do something inconsistent...

Comment on lines +32 to +33
def __init__(self, processor_ids: Union[int, Iterable[int], None],
link_ids: Union[int, Iterable[int], None],
Copy link
Member

Choose a reason for hiding this comment

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

Can't quite remember if we prefer Optional rather than Union[..., None], though if the comment below is taken into account, this would of course go away...

@Christian-B
Copy link
Member Author

replaced by #250

@Christian-B Christian-B closed this Apr 9, 2024
@Christian-B Christian-B deleted the remove_18 branch April 9, 2024 11:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants