mesa icon indicating copy to clipboard operation
mesa copied to clipboard

Addition of inner_radius parameter in vision for get_neighbor[...] functions

Open hildobby opened this issue 6 years ago • 6 comments

Sometimes when using a large radius, the code computes for unnecessary neighbors. For optimisation purposes, usinginner_radius allows higher precision of the desired neighbors. This is done by only looking from a certain radius and up instead of from a given origin.

hildobby avatar Feb 03 '20 19:02 hildobby

Codecov Report

Merging #793 into master will decrease coverage by 0.16%. The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #793      +/-   ##
==========================================
- Coverage   84.64%   84.48%   -0.17%     
==========================================
  Files          17       17              
  Lines        1029     1031       +2     
  Branches      169      170       +1     
==========================================
  Hits          871      871              
- Misses        127      128       +1     
- Partials       31       32       +1
Impacted Files Coverage Δ
mesa/space.py 93.02% <33.33%> (-0.63%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 4cfd991...8498eea. Read the comment docs.

codecov[bot] avatar Feb 03 '20 19:02 codecov[bot]

This seems like a useful addition. A couple quick requests:

  • It looks like this was developed just before the recent change that added type annotations to the space module. Can you merge the most recent master into your branch, so that accepting this PR doesn't remove the recent changes?

  • The name and purpose of inner_radius aren't very clear. Can you add a docstring which explains when/how to use it?

dmasad avatar Feb 05 '20 13:02 dmasad

@dmasad I added type annotations back to space.py to match the recent changes and also added a docstring on inner_radius where it can be used as a parameter. Is this fine?

hildobby avatar Feb 05 '20 15:02 hildobby

Looks like a good addition, but could you add a unit test for this function? Just to make sure future changes don't break your function and produce ugly bugs

Corvince avatar Feb 06 '20 08:02 Corvince

@Corvince I will try some more but I have little to no experience with writing tests. Could you maybe help me or give me some pointers?

hildobby avatar Feb 09 '20 17:02 hildobby

Sure! To run tests you must have pytest installed. Then, when you are in your mesa directory, run py.test tests to run all the tests and confirm pytest is working. To run a single test-file just use for example py.test tests/test_grid.py. Depending on which editor or IDE you are using you might also be able to run and/or debug tests from there (I am using vscode where you can debug single test cases)

This file (test_grid.py) is also where your tests should go. At the top of the file you see the test grid set-up that is currently used. You can then read further on how the test cases are set-up and tested. A test of your function should either go under test_neighbors or within its own function.

I must say the way unit tests are currently designed might not be the best way for a comprehensive unit tests. So you are also free to come up with another solution!

Corvince avatar Feb 13 '20 08:02 Corvince