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

Change in GetStructrualPlasticityStatus format #775

Merged
merged 7 commits into from Jul 11, 2017
Merged

Conversation

@sdiazpier
Copy link
Contributor

sdiazpier commented Jul 2, 2017

This PR addresses issue #260 by changing the call format for GetStructuralPlasticityStatus. Now the parameter for the call is optional and can include a string defining the elements of interest from the status dictionary. The call still returns the dictionary containing the status filtered by the parameter string. A small test was also added.

Copy link
Contributor

heplesser left a comment

@sdiazpier Thank you for your pull request! I have commented on a few issues; you also need to fix a number of PEP8 issues to make Travis happy.

@@ -234,14 +234,20 @@ def SetStructuralPlasticityStatus(params):


@check_stack
def GetStructuralPlasticityStatus(params):
def GetStructuralPlasticityStatus(keys = None):

This comment has been minimized.

@heplesser

heplesser Jul 2, 2017 Contributor

No spaces around = in parameter lists.

@@ -234,14 +234,20 @@ def SetStructuralPlasticityStatus(params):


@check_stack
def GetStructuralPlasticityStatus(params):
def GetStructuralPlasticityStatus(keys = None):
"""Get the current structural plasticity parameters for the network
simulation.

This comment has been minimized.

@heplesser

heplesser Jul 2, 2017 Contributor

Please document the keys parameter.

if keys is None:
return d
elif is_literal(keys):
return d[keys]

This comment has been minimized.

@heplesser

heplesser Jul 2, 2017 Contributor

Here, keys can only be a single key, so why the plural? And wouldn't it make more sense to allow a list of keys, as in GetStatus?

This comment has been minimized.

@sdiazpier

sdiazpier Jul 2, 2017 Author Contributor

I left it plural but allow now a list like in GetStatus :)

elif is_literal(keys):
return d[keys]
else:
raise TypeError("keys should be either empty or a string")

This comment has been minimized.

@heplesser

heplesser Jul 2, 2017 Contributor

"should" is not a good term here, as it implies a recommendation, not a requirement, while we have strict requirements on the type of keys. Use "must" instead.

@@ -60,9 +62,10 @@ def suite():
test_suite.addTest(test_growth_curves.suite())
test_suite.addTest(test_sp_manager.suite())
test_suite.addTest(test_synaptic_elements.suite())
test_suite.addTest(test_disconnect.suite())
#test_suite.addTest(test_disconnect.suite())

This comment has been minimized.

@heplesser

heplesser Jul 2, 2017 Contributor

Why are you commenting out the disconnection test? Either activate it again or delete the line entirely.

This comment has been minimized.

@sdiazpier

sdiazpier Jul 2, 2017 Author Contributor

This is a mistake, I will remove the comment it now.

'''
Structural Plasticity GetStatus Test
-----------------------
This test shows how to use the GetStructuralPlasticityStatus function

This comment has been minimized.

@heplesser

heplesser Jul 2, 2017 Contributor

The test should rather show that the function works correctly. That it also illustrates how to use the function is rather a (welcome) side-effect.

This comment has been minimized.

@sdiazpier

sdiazpier Jul 2, 2017 Author Contributor

Ok, will correct the text

all = nest.GetStructuralPlasticityStatus()
print (all)
assert('structural_plasticity_synapses' in all)
assert('structural_plasticity_update_interval' in all)

This comment has been minimized.

@heplesser

heplesser Jul 2, 2017 Contributor

Why are you not testing the values here?

This comment has been minimized.

@sdiazpier

sdiazpier Jul 2, 2017 Author Contributor

I just want to know that the elements have been gathered by the call to get status but I can test the values too :)

@heplesser heplesser requested a review from jakobj Jul 2, 2017
@heplesser heplesser added this to the NEST 2.12.1 milestone Jul 2, 2017
Copy link
Contributor

heplesser left a comment

@sdiazpier I approve, conditional on Travis passing. Problems there all seem to be PEP8 issues, mostly due to tabs instead of spaces.

sdiazpier added 4 commits Jul 3, 2017
Copy link
Contributor

abigailm left a comment

just a typo, otherwise lgtm

"""
Structural Plasticity GetStatus Test
-----------------------
This test the functionality of the GetStructuralPlasticityStatus

This comment has been minimized.

@abigailm

abigailm Jul 4, 2017 Contributor

this tests

@abigailm abigailm merged commit bbcdd91 into nest:master Jul 11, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
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.