SpiNNakerManchester / PACMAN

Partition and Configuration Manager for SpiNNaker
Apache License 2.0
9 stars 7 forks source link

ordered_covering compressor does not check 1023 entries #374

Closed Christian-B closed 2 years ago

Christian-B commented 3 years ago

In ordered_covering compressor If router_table_compression_target_length is set to None it compresses as much as possible but then does not check if the result is less than 1023

Also bring the constant 1023 out of HostBasedBitFieldRouterCompressor

Christian-B commented 3 years ago

Related to https://github.com/SpiNNakerManchester/SpiNNFrontEndCommon/issues/793

So removal of target length using 1023 or as far as possible may be a better fix.

rowleya commented 3 years ago

Note that:

  1. The on-machine compressors probably don't actually check 1023, but rather try to load the table and if this fails at the final step, this is a failure. This means that it will work regardless of how many system entries there are.
  2. The on-host compressors will similarly fail when the table is eventually loaded (though this might be considered "too late").
  3. SpiNNaker-2 will have more entries (16K entries) per router, so it may be better to use "target length" to allow the algorithm to remain the same (although with 16K entries it remains to be seen how much compression will actually be needed).
Christian-B commented 3 years ago

Even we go with the plan to remove the USER provided target length there would still be a system provided minimum length. Which is better than demanding the user changes the cfg for SpiNNaker-2 values

Currently if the ordered_covering finds it can get more than the user set number of entries but less than the system required number it FAILS!

A better suggestion is in https://github.com/SpiNNakerManchester/SpiNNFrontEndCommon/issues/793 The change would be that is the user wants to compress more than that he sets router_table_compress_as_far_as_possible to true rather than lowering router_table_compression_target_length

Note: ordered_covering is Mundy's compressor on host so highly likely to ever be used for more than historical comparisons and testing.

rowleya commented 3 years ago

For users, they probably also don't know if they want to compress as far as possible either, but I see the point. I guess the option to compress as far as possible is useful when a downstream algorithm wants to make use of the space left (though I don't think we have one currently that doesn't already do its own compression).

Christian-B commented 2 years ago

already fixed.