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

Added test for spatial kernels in topology (depends on #823). #929

Merged
merged 8 commits into from Apr 19, 2018

Conversation

@heplesser
Copy link
Contributor

@heplesser heplesser commented Apr 17, 2018

This PR adds test for spatial kernels to NEST, based on code from Daniel Hjertholm's master thesis.

It can only be merged once #823 is merged, since it requires the gamma kernel.

@babsey Could you also review?

Copy link
Contributor

@stinebuu stinebuu left a comment

Great to have a test for the kernels, that is certainly needed! This looks mostly good, however I do wonder if the plotting really should be included. I do see the need for it if new kernels are included, but the test file is already quite long, the SpacialTester class is in particular long, and at least as far as I am aware, we usually reserve plotting etc for examples and not for tests?

dim : Dimensions (2 or 3)
L: Side length of area / volume.
N: Number of nodes.
open_bc : Network with open boundary conditions

This comment has been minimized.

@stinebuu

stinebuu Apr 18, 2018
Contributor

Could you align the comments of L and N with the rest of the comments?

This comment has been minimized.

@heplesser

heplesser Apr 18, 2018
Author Contributor

Done.

def _reset(self, seed):
'''Reset simulator and seed PRNGs.'''

pass

This comment has been minimized.

@stinebuu

stinebuu Apr 18, 2018
Contributor

This is where I got confused about the undefined functions :)

This comment has been minimized.

@heplesser

heplesser Apr 18, 2018
Author Contributor

All gone.


class SpatialTester(object):
'''Tests for spatially structured networks.'''

This comment has been minimized.

@stinebuu

stinebuu Apr 18, 2018
Contributor

Could you add a comment somewhere explaining that this is a base class? I was quite confused about why the class contained functions that was not implemented, I hadn't seen that the file contains several classes that builds on this and that they have the implementations of the functions.

This comment has been minimized.

@heplesser

heplesser Apr 18, 2018
Author Contributor

Removed the subclass.

-------------
Unnormalized PDF at distance D.
'''

This comment has been minimized.

@stinebuu

stinebuu Apr 18, 2018
Contributor

This function is quite long and cluttered. I had a very quick look in the thesis, but when I couldn't find a total definition of the PDF I gave up. Looking at this function, it does make sense that the definition of the PDF is scattered or not written down explicitly in the thesis, as it has so many different constraints/conditions, but I have to admit that I have not looked at the math here in detail. Seems to be working though, as the test pass! Some comments might be helpful, but it might just make the function more dense.

This comment has been minimized.

@heplesser

heplesser Apr 18, 2018
Author Contributor

I don't have more details at the moment, but I will add documentation when it becomes available (as publication). I hope that is good enough for now.

heplesser added a commit that referenced this pull request Apr 18, 2018
Add gamma kernel in topology. Test will follow with #929.
@heplesser
Copy link
Contributor Author

@heplesser heplesser commented Apr 18, 2018

@stinebuu I have restructured and simplified the code. I decided to keep the plotting in for now (but split it out into a separate subclass), since code placement would otherwise have become a bit problematic when supporting a test without plotting and an example with plotting. I have also merged all recent changes from master, so #823 with the gamma kernel is now included.

@babsey
Copy link
Contributor

@babsey babsey commented Apr 18, 2018

Thank you for asking me to review it.
I had a great opportunity to review 790 lines. Please forgive me for taking some time. 😄

Some technical points concerning me are:

  • DEBUG_MODE is on. I assume it should be off.
  • the code has functions (sqrt,exp) from math and some from numpy
  • np.sum could be faster than sum.

I do not consider the correct outcome of the spatial testing because I trust @heplesser and @stine that this testing suite is working.

@heplesser heplesser requested a review from babsey Apr 18, 2018
Copy link
Contributor

@stinebuu stinebuu left a comment

@heplesser thank you, the code is a lot more readable to me now, and I like the solution with the plotting. I have two small comments, but otherwise approve.

# for debugging
from mpl_toolkits.mplot3d import Axes3D
import matplotlib.pyplot as plt
# make sure we can open a window; DISPLAY may not be set

This comment has been minimized.

@stinebuu

stinebuu Apr 18, 2018
Contributor

What does DISPLAY refer to?

This comment has been minimized.

@babsey

babsey Apr 18, 2018
Contributor

It's an environment variable for X11 programs. You can check yours in terminal.
echo $DISPLAY

In this case matplotlib needs to have DISPLAY environment for opening a window.

This comment has been minimized.

@stinebuu

stinebuu Apr 18, 2018
Contributor

Nice!

except:
PLOTTING_POSSIBLE = False

# If False, tests will be run; otherwise, as single case will be

This comment has been minimized.

@stinebuu

stinebuu Apr 18, 2018
Contributor

I assume you mean 'a single case...' and not 'as single case..'!

This comment has been minimized.

@heplesser

heplesser Apr 18, 2018
Author Contributor

Fixed.

@heplesser
Copy link
Contributor Author

@heplesser heplesser commented Apr 18, 2018

@babsey I have now also explicitly added as a reviewer, so you get a nice "Submit review" button with "comment/require changes/approve" radio buttons to click :).

  • DEBUG_MODE is off now, I accidentally pushed a version with it on first.
  • I will use math everywhere except where I really need NumPy. The complicated case distinctions in _pdf operate on scalars only, and for them math versions are faster than np versions of mathematical functions.
  • Since the operands to sum() are lists, not arrays, I think np.sum() would not make much of a difference. Anyways, the main optimization potential for these tests is in _pdf and _cdf, but that will need to wait till another time.

I will push in a little while.

variance_num_targets = sum([p * (1. - p) for p in ps])

if variance_num_targets == 0:
return math.nan, 1.0

This comment has been minimized.

@babsey

babsey Apr 18, 2018
Contributor

math.nan is not working. Change it to np.nan.

This comment has been minimized.

@heplesser

heplesser Apr 18, 2018
Author Contributor

Right, math.nan was introduced with Py 3.5 only.

plt.plot(x, y, color='black', linewidth=3, label='Theory',
zorder=1)
plt.hist(self._target_dists, bins=bins, histtype='step',
linewidth=1, density=True, color='red',

This comment has been minimized.

@babsey

babsey Apr 18, 2018
Contributor

the argument density works with matplotlib 2.2 but for older version it would give an error. Older versions has this argument but it called normed.

This comment has been minimized.

@heplesser

heplesser Apr 18, 2018
Author Contributor

Ok, reverted to normed for backward compatibility.

(math.sqrt(2) * math.log((.1 - 0) / 1))},
'gaussian': {'p_center': 1., 'sigma': self._L / 4.,
'mean': 0., 'c': 0.},
'gamma': {'kappa': 3., 'theta': self._L / 4.}}

This comment has been minimized.

@babsey

babsey Apr 18, 2018
Contributor

I tested it, an adequate parameter set for gamma would be
'gamma': {'kappa': 4., 'theta': self._L / 20.}}

This comment has been minimized.

@heplesser

heplesser Apr 18, 2018
Author Contributor

In which sense adequate? Wouldn't essentially any parameter set with kappa != 1 be useful?

This comment has been minimized.

@babsey

babsey Apr 18, 2018
Contributor

You are right, it is a personal view.

@babsey
Copy link
Contributor

@babsey babsey commented Apr 18, 2018

The Travis passed. I also approve the merger.

@heplesser heplesser merged commit ea137fb into nest:master Apr 19, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@heplesser heplesser deleted the heplesser:spatial_kernel_test branch Apr 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.