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

100% coverage #185

Merged
merged 17 commits into from Feb 22, 2019

Conversation

Projects
None yet
3 participants
@hunse
Copy link
Contributor

hunse commented Feb 6, 2019

Get 100% coverage in codecov.

  • Cover all BuildErrors and other exceptions
  • Remove unused code
  • Test code that should not fail in tests (e.g. chip connection/communication) using mocks
  • Cover any remaining uncovered lines, or mark as "no cover" (e.g. failed imports)

@hunse hunse force-pushed the coverage100 branch 7 times, most recently from 5ee4dcc to 6a3b99d Feb 6, 2019

@hunse

This comment has been minimized.

Copy link
Contributor Author

hunse commented Feb 8, 2019

OK, I think I've got everything covered, except for the few cases that we've explicitly talked about.

Currently, this is still a lot of commits. I've organized them into added tests first, then code removals, then other stuff. I've kept them separate for now (I'm thinking it will be easier to review that way), but they can mostly be squashed before merging (maybe squashing all the tests into one commit, and most of the removals into another). The two removals we should keep separate IMO are the first two in the order right now: e29050a and dfa4da5. The first removes the NIF neurons and replaces them with SpikingRectifiedLinear for our node encoding. The second gets rid of probes from LoihiInput, which we don't use right now, but may want to add back in in the future to be able to probe parts of host->chip connections.

I also think we should keep the "other stuff" mostly separate (i.e. things that are not tests or removals), since they're all fairly different changes and some have potential functional consequences.

This is also based off #140 right now (to fix the tests), so we should merge that first.

@hunse

This comment has been minimized.

Copy link
Contributor Author

hunse commented Feb 15, 2019

One thing we haven't discussed is conftest.py. It technically has one uncovered line. I would recommend removing it from coverage entirely.

@drasmuss drasmuss force-pushed the coverage100 branch from c66bfc7 to 6ad3949 Feb 19, 2019

@drasmuss
Copy link
Contributor

drasmuss left a comment

Pushed some fixups, then just one other question about test coverage.

Show resolved Hide resolved nengo_loihi/emulator/interface.py
Show resolved Hide resolved pytest.ini
Show resolved Hide resolved nengo_loihi/hardware/builder.py Outdated

@drasmuss drasmuss force-pushed the coverage100 branch from d9793ba to f40bf9d Feb 19, 2019

Show resolved Hide resolved nengo_loihi/tests/test_inputs.py Outdated

@drasmuss drasmuss force-pushed the coverage100 branch 2 times, most recently from d738b23 to 4f40b08 Feb 19, 2019

@hunse hunse force-pushed the coverage100 branch from 348007d to cf0b95e Feb 20, 2019

@drasmuss
Copy link
Contributor

drasmuss left a comment

Looks good to me!

Need to add changelog entries before merging, but Eric is working on that now.

@hunse hunse force-pushed the coverage100 branch from 9c53d1e to acee2f3 Feb 20, 2019

@hunse

This comment has been minimized.

Copy link
Contributor Author

hunse commented Feb 20, 2019

I've added changelog entries. There were only four commits that I thought deserved them.

Check LoihiBlock probe target
This was accidentally lost in 9b68839 during refactoring.

@tbekolay tbekolay force-pushed the coverage100 branch 2 times, most recently from 135a212 to 7716ba6 Feb 21, 2019

hunse added some commits Feb 5, 2019

Expand test coverage
- Test validation BuildErrors
- Test splitter errors and other untested lines
- Test ProbeDict interface
- Test Simulator interface
- Test Passthrough node removal transform errors
- Test LoihiBlock.compartment configuration errors
- Test error for no appropriate weight exponent
- Test error for no appropriate bias scaling
- Test loihi_rates with non-Loihi neuron
- Test LoihiBlock too big to fit on one core
- Test ChipReceiveNode run error
- Test ensemble building errors
- Test discretize noise amplitude warnings
- Test Model.build_callback
- Test zero activity error in connection
- Test chip->host connections defined by points
- Test block.py object string functions
- Test nxsdk_objects.py object strings
- Test PESModulatoryTarget interface
- Test emulator states dtype errors
- Test Loihi simulation exception handling
  Currently, this exception handling does not work properly, so this
  test hangs. We need to keep track of how many steps have executed,
  and perform the exact number of chip reads/writes required to
  finish the simulation.
- Test unrecognized probe target type error
- Ensure that we target specific warnings in tests
Remove NIF neuron type
This was only used for generating spikes to send to the chip,
which can be accomplished equally well with SpikingRectifiedLinear.
Remove probes from LoihiInput
LoihiInput is always on the chip, connecting in from a Node.
If a user probes the node, though, the probe will happen off-chip.
So it's currently impossible to probe these on-chip. This may be
possible in the future if/when we set up connection probing
for host->chip connections.

hunse and others added some commits Feb 6, 2019

Remove unused code
- Remove StdpProfile and StdpPreProfile
  While we configure these in the hardware builder, we always configure
  them the same for PES, so these configuration classes are not needed.
- Remove unused axon names from LoihiBlock
- Remove unused SpikeInput.clear_spikes
- Remove unused Synapse.set_diagonal_weights
- Remove unhittable signal probe function error
- Remove unused helper functions in IterableState
- Remove unused default for Interface.chip2host
- Remove unused HardwareInterface.print_cores
- Remove unused Block.cores attribute
Simplify code to avoid uncovered lines
Mostly, this is acknowledging cases that can never happen.

- Earlier error if using direct neurons in builder.
- Make sure increment size assert gets executed every time,
  and give it a better message.
- Simplify connection builder NotImplemented block to not
  check specially for nodes.
- Spike probe targets can only be LoihiBlock, so just assert this.
- Only warn about noise amplitude if it's enabled, to avoid
  unnecessary warnings.
- Check that all synapses on a core are either learning or
  not learning (but not a mix of both), because our current
  allocator does not support this. When we do, we'll need
  to properly test it, including the uncovered lines that are
  commented out in this commit.
- Can never have zero axons in hardware builder, so just assert it.
Test board builder pop type errors
Also changed the error messages to make them distinguishable
and able to report non-integer pop types (which are not supported).

Replace untested pop_type error with assert
Test errors for failing to connect to the board
As part of this, I discovered that if `n2Board` is None, we
raise a SimulationError but still try to call `n2Board.disconnect`
if we're in a context manager. So now we only call that if
`n2Board` is not None.
Test simulator picking up where it left off
By running first for some time, then again for more time.
This tests a number of previously uncovered lines.
Clarify negative cx_base value for ignoring axon
A negative cx_base value is sometimes helpful to indicate an axon
that is not required when making convolutional connections. The chip
itself does not support negative cx_base values, so we just remove
this axon when building the model. We used to use a value of -2048
to indicate this, but this led to the emulator misleadingly allowing
smaller negative values. Now, we just use -1 to indicate an unused
axon, and give an error for other values.

Also added a test of negative cx_base values working as expected.
Fix noise for small exponents
The chip allows noise exponents down to zero; this now works on the
emulator as well.

However, a known abnormality in shifting on the chip means that
noise exponents less than 7 act as one greater than they should.
This commit addresses that problem by subtracting one from
weight exponents before putting them on the chip.

We also improve the test for noise, so that it tests different
levels of noise and asserts that the statistics of the resulting
Wiener process are as expected.
Mark specific lines as not covered
These lines depend on how we have the tests set up and hitting
them is not important.
Remove test_input_node xfail
It now passes reliably on chip, likely due to
new interneurons.
Reorganize test files
Reorganize the test files to more closely
reflect the non-test file structure.

- Moved test_communication.py -> test_connection.py
- Moved test_ens_on_host.py -> test_connection.py
- Moved test_model.py -> test_simulator.py
- Moved test_multiple.py -> test_node.py
- Moved test_precompute.py -> test_simulator.py
- Moved test_radius.py -> test_ensemble.py
- Moved test_snips.py -> test_simulator.py
- Moved emulator/tests/test_emulator.py ->
  emulator/tests/test_interface.py
- Moved hardware/tests/test_simulator.py ->
  hardware/tests/test_interface.py
- Moved test_simulator.py::test_snip_input_count ->
  hardware/tests/test_interface.py

@tbekolay tbekolay force-pushed the coverage100 branch from 7716ba6 to 40981e4 Feb 22, 2019

@tbekolay
Copy link
Member

tbekolay left a comment

History looks great, I only had to fix one test (test_split_conv2d_transform_error) post-rebase as it changed due to #142. Merging once CI finishes.

@tbekolay tbekolay merged commit 40981e4 into master Feb 22, 2019

2 checks passed

codecov/patch 100% of diff hit (target 100%)
Details
codecov/project 99.34% (+3.64%) compared to 9cef210
Details

@tbekolay tbekolay deleted the coverage100 branch Feb 22, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.