Fix C SA LocationConstraints

mossblaser commented 6 years ago

@mundya Hope you don't mind coming "out of retirement" to have a quick look over this PR?

This PR fixes a (really extremely embarrassing) bug in the Python interface to the C Kernel for the simulated annealing based placement algorithm. The C interface (as clearly documented in its header) requires that vertices are listed such that moveable vertices are listed first. The Python side of this, however, somehow never actually implemented this sorting step... In this PR, the test suite is fixed to find the bug and the implementation is fixed to add the sort.

Thanks very much to @robert-hardwick in project-rig/rig_c_sa#7 for spotting this bug. I'm really sorry it appeared. Its funny that two years on I could remember that "of course you must sort the vertices" but somehow didn't while actually writing the software... How very embarrassing.

@robert-hardwick Would you be able to check out this branch and check it fixes your issue?

[x] Fix test suite to find SA bug
[x] Fix SA wrapper
[x] Fix new flake8 errors
[x] Fix new machine_controller test warnings
[x] Fix new ordered covering test warnings (assigned @mundya)
[x] Run tests against reference hardware setup
[ ] Diagnose source of failing fan indicator error

mossblaser commented 6 years ago

(NB: Failing tests resulting from non-contactable SpiNNaker board will need to be fixed by prodding someone in the SpiNNaker team to provide a test machine to run the CI against...)

robert-hardwick commented 6 years ago

@mossblaser I have checked out this branch and it does indeed fix the bug. The code change is very similar to what I had done to temporarily fix it.

Thanks for the very prompt response on this. To be honest I wasn't expecting it to get looked at straight away.

Let me know if there is anything else I can help with.

mundya commented 6 years ago

@mossblaser Do we want to prod someone at Manchester for a board against which the CI can run? Or, @robert-hardwick do the test suites run against a board you have access to? Thanks!

mossblaser commented 6 years ago

@mossblaser Do we want to prod someone at Manchester for a board against which the CI can run?

I was going to but noticed that travis is barfing about something which looks like a Pytest API change so was going to try and fix that first. (For all I know, that random collection of proxies and idle SpiNNaker boards are still alive, yet!)

mundya commented 6 years ago

Note to self: read error logs first!

robert-hardwick commented 6 years ago

I can do yes. Can somebody show me how to set the ip address for the test-suites to run against?

mossblaser commented 6 years ago

Can somebody show me how to set the ip address for the test-suites to run against?

You should be able to run the tests using

$ pip install -r requirements-test.txt
$ py.test tests \
        --spinnaker SPINN_HOSTNAME 8 8 --spinn5 \
        --bmp BMP_HOSTNAME

Ideally you should have a Spnn5 board which is on its own, is powered off and has a connection to both its SpiNNaker port and BMP port.

There are a few more details about running the test suite in DEVELOP.md but do ask if you're stuck!

Thanks for your help!

robert-hardwick commented 6 years ago

I have a spinn5 board, but i'm not easily able to use the BMP port ( i probably could do with some time, let me know if this is important ). The tests do pass without using the --bmp option. I do however get 4 warnings, but these are the same as on the master branch so i'm assuming them to be normal.

$ pytest tests --spinnaker 192.168.X.X --spinn5 --no-boot

1514 passed, 8 skipped, 4 warnings in 13.63 seconds

On a side note, without the --no-boot option and with the device unbooted the tests fails but are unable to display the results which looks like the pytest API change which was refferred to before.

rt INTERNALERROR> call.excinfo.type is not _pytest.skipping.XFailed: INTERNALERROR> AttributeError: 'module' object has no attribute 'XFailed'

On Tue, May 8, 2018 at 10:39 AM, Jonathan Heathcote < notifications@github.com> wrote:

Can somebody show me how to set the ip address for the test-suites to run against?

You should be able to run the tests using

$ pip install -r requirements-test.txt $ py.test tests \ --spinnaker SPINN_HOSTNAME 8 8 --spinn5 \ --bmp BMP_HOSTNAME

Ideally you should have a Spnn5 board which is on its own, is powered off and has a connection to both its SpiNNaker port and BMP port.

There are a few more details about running the test suite in DEVELOP.md https://github.com/project-rig/rig/blob/master/DEVELOP.md#testing but do ask if you're stuck!

Thanks for your help!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/project-rig/rig/pull/270#issuecomment-387345398, or mute the thread https://github.com/notifications/unsubscribe-auth/APhKLYgr8pUNkSveV6zeNI8rApU06lj7ks5twWe3gaJpZM4T2G3q .

mossblaser commented 6 years ago

Thanks for checking that out! I'll try and get a look at the pytest issue when I've got some time (may be tomorrow's commute...).

If you're alright using your own patch/this branch in the meantime I'll postpone pushing a new version until the test suite bug is worked out.

Thanks again for all your efforts and best of luck. I'd enjoy hearing about the work you're getting up to once its all a bit more public!

robert-hardwick commented 6 years ago

No problems.

No rush, i'm fine to keep working with the patched branch for the time being.

Of course, will let you know if/when we publish anything. Still early days yet.

Rob (mindtrace)

On Tue, May 8, 2018 at 1:54 PM, Jonathan Heathcote <notifications@github.com

wrote:

Thanks for checking that out! I'll try and get a look at the pytest issue when I've got some time (may be tomorrow's commute...).

If you're alright using your own patch/this branch in the meantime I'll postpone pushing a new version until the test suite bug is worked out.

Thanks again for all your efforts and best of luck. I'd enjoy hearing about the work you're getting up to once its all a bit more public!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/project-rig/rig/pull/270#issuecomment-387392429, or mute the thread https://github.com/notifications/unsubscribe-auth/APhKLfyEXexRL5JvhJEieTRiEQs-NZCQks5twZWVgaJpZM4T2G3q .

mundya commented 6 years ago

@mossblaser There are (at least under Python 3.5) also some doctest failures and more flake8 failures. I'll try and snag the routing table thing and the doctest thing this weekend.

mossblaser commented 6 years ago

Thanks @mundya, re:

more flake8 failures

I've fixed these now. I'll take a look at the doctests in a minute...

Many thanks to @lplana for sorting out a SpiNNaker board for running the tests on!

The suite falis with:

_____________________ TestBMPControllerLive.test_read_adc ______________________
self = <test_bmp_controller.TestBMPControllerLive object at 0x7f16b963af90>
live_controller = <rig.machine_control.bmp_controller.BMPController object at 0x7f16b963a210>
    @pytest.mark.order_after("bmp_power_cycle")
    def test_read_adc(self, live_controller):
        # Read the ADC values and simply check they're within realistic ranges
        adc = live_controller.read_adc()
        assert 1.1 < adc.voltage_1_2a < 1.3 or -0.1 < adc.voltage_1_2a < 0.1
        assert 1.1 < adc.voltage_1_2b < 1.3 or -0.1 < adc.voltage_1_2b < 0.1
        assert 1.1 < adc.voltage_1_2c < 1.3 or -0.1 < adc.voltage_1_2c < 0.1
        assert 1.7 < adc.voltage_1_8 < 1.9 or -0.1 < adc.voltage_1_8 < 0.1
        assert 3.2 < adc.voltage_3_3 < 3.4 or -0.1 < adc.voltage_3_3 < 0.1
        assert 10.0 < adc.voltage_supply < 14.0
        assert 5.0 < adc.temp_top < 100.0
        assert 5.0 < adc.temp_btm < 100.0
        assert adc.temp_ext_0 is None or 5.0 < adc.temp_ext_0 < 100.0
        assert adc.temp_ext_1 is None or 5.0 < adc.temp_ext_1 < 100.0
>       assert adc.fan_0 is None or 0.0 < adc.fan_0 < 10000.0
E       assert (0.0 is None or 0.0 < 0.0)
E        +  where 0.0 = ADCInfo(voltage_1_2c=1.2384033203125, voltage_1_2b=1.2384033203125, voltage_1_...=28.0, temp_btm=31.625, temp_ext_0=None, temp_ext_1=None, fan_0=0.0, fan_1=0.0).fan_0
E        +  and   0.0 = ADCInfo(voltage_1_2c=1.2384033203125, voltage_1_2b=1.2384033203125, voltage_1_...=28.0, temp_btm=31.625, temp_ext_0=None, temp_ext_1=None, fan_0=0.0,collected 23 items

Which appears to be due to one of the following possibilities:

The BMP now reports absent fans with a "0" rather than a "-1"
The BMP now does more fancy speed control and this board's fan simply isn't switched on
The fan isn't working

I'm following this up and will determine what to do about this failing test accordingly...

mossblaser commented 6 years ago

@mundya I've fixed the doctest failure but I'm afraid the remaining warnings from the doctest run relate to the same orthogonality warnings so I'll leave those up to you to amend if you don't mind!

mundya commented 6 years ago

Thanks, @mossblaser - the last commit I've added catches the one warning which shows up on my machine. We'll see whether that makes Travis happy otherwise I'll hunt out the others.

mossblaser commented 6 years ago

@mundya your changes LGTM (minus the need to update DEVELOP.md which I've done)

A couple of test fixes myself (following a chat with Luis about the fan speeds) and hopefully we should soon be green again!

mundya commented 6 years ago

All LGTM... merge at will!

project-rig / rig

Fix C SA LocationConstraints #270