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

PyNEST: pep8 compliance and Numpy-formatted docstrings. #263

Merged
merged 9 commits into from Apr 22, 2016

Conversation

Projects
None yet
4 participants
@flinz
Contributor

flinz commented Mar 7, 2016

This PR came about due to #188 and #257.

Major changes:

  • added numpy standardized docstring (see here for specs) to all functions in pynest/nest, excluding pynest/nest/tests in preparation for autogeneration of documentation
  • pep8 compliant formatting for almost all python files in pynest/nest, so the nest directory now checks out as almost clean under pep8 (see issues below)
  • raster_plot.py has been slightly rewritten to simplify redundant keyword arguments

Remaining issues

  • I tried as best I could to infer missing parameter types etc for the doc-strings, there might be some slight "mistypings".
  • Problem with pynest/nest/__init__.py scipy bugfix: Right now pep8 checks out as almost clean, except the 4 errors below. This is due to the fact that the imports of pynestkernel and hl_api_helper come after the scipy bugfix (see here). Does anybody know more about this scipy import issue? Can we move the 4 import lines to the top of the file? I do not see scipy being imported anywhere there, so in my opinion this should be safe to do.
% pep8 nest
nest/__init__.py:57:1: E402 module level import not at top of file
nest/__init__.py:58:1: E402 module level import not at top of file
nest/__init__.py:60:1: E402 module level import not at top of file
nest/__init__.py:61:1: E402 module level import not at top of file

flinz added some commits Mar 4, 2016

pep8 compliance (+6 squashed commits)
Squashed commits:
[576ce0d] pep8 error fixed
[0f4b50d] pep8 compliance; changed NestError -> NESTError
[298c5b0] pep8 compliance of raster_plot. Moved kwargs definitions to make_plot; Passing kwargs to make_plot from all others;
[605efae] pep8 compliance; could not fix 4 E402 errors, due to module-wide imports later in the file which are necessary as workarounds.
[5d4e813] fixed bug that was introduced by pep8 rewrite
[a6a47fa] added helper to correctly format deprecation warnings, respecting linebreaks, without relying on textwrap.dedent()
@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Mar 8, 2016

Contributor

@flinz Thanks for your work! At the moment, the checks fail due to some copyright header problem---could you check that?

Import order, especially wrt SciPy/NumPy has been a problem for a long time, and one sensitive to a number of details, if I remember correctly. I would tend not to fiddle with the order of imports just to make PEP8 happy, in particular in an __init__.py that is not part of the public Python interface of NEST. I think this is a place where we probably should suppress the PEP8 warning.

Contributor

heplesser commented Mar 8, 2016

@flinz Thanks for your work! At the moment, the checks fail due to some copyright header problem---could you check that?

Import order, especially wrt SciPy/NumPy has been a problem for a long time, and one sensitive to a number of details, if I remember correctly. I would tend not to fiddle with the order of imports just to make PEP8 happy, in particular in an __init__.py that is not part of the public Python interface of NEST. I think this is a place where we probably should suppress the PEP8 warning.

@flinz

This comment has been minimized.

Show comment
Hide comment
@flinz

flinz Mar 8, 2016

Contributor

@heplesser copyright header errors should be fixed now.
I also moved all imports in __init__.py back to the original order (I had already moved two) just to be sure.

Contributor

flinz commented Mar 8, 2016

@heplesser copyright header errors should be fixed now.
I also moved all imports in __init__.py back to the original order (I had already moved two) just to be sure.

Show outdated Hide outdated pynest/nest/__init__.py
"""Convenience function for executing the sequence
sli_push(args); sli_run(s); y=sli_pop().
The function takes an

This comment has been minimized.

@jougs

jougs Mar 8, 2016

Contributor

Something is missing here. Probably, the sentence can just be deleted, as the thing with the arbitrary number of arguments is written below.

@jougs

jougs Mar 8, 2016

Contributor

Something is missing here. Probably, the sentence can just be deleted, as the thing with the arbitrary number of arguments is written below.

synapse_model : str, optional
Only connections with this synapse model are returned
synapse_type : str, optional
Only connections with this synapse type are returned

This comment has been minimized.

@jougs

jougs Mar 8, 2016

Contributor

I'd add the fact that this is the same as synapse_model. On the other side, the function is deprecated since long and can as well be deleted right after this PR is merged.

@jougs

jougs Mar 8, 2016

Contributor

I'd add the fact that this is the same as synapse_model. On the other side, the function is deprecated since long and can as well be deleted right after this PR is merged.

This comment has been minimized.

@jougs

jougs Mar 8, 2016

Contributor

Forget this! I just saw that it is mentioned below. Nevermind.

@jougs

jougs Mar 8, 2016

Contributor

Forget this! I just saw that it is mentioned below. Nevermind.

flinz added some commits Mar 8, 2016

Merge branch 'master' into pynest-docs
* master:
  Formatting fixes.
  Small fixes in tests.
  Added checks against negative or infinite simulation time.
  Refactored Time object update to static helper method.
  Simtime now rounded up, improved comment on it.
  If GLS is available, use GSL RNG and thus allow
  Fixing #264: Simulate now raises exception if simulation time is not multiple of the resolution. SetStatus on devices do the same if start, stop, origin are not multiples of the resolution. Some tests needed minor adaptation.
  Add a test that checks for return type of RDV
  Add an `is_discrete` entry to the RDV Status dict
  BugFix: The RandomFunction either returns
  Improves documentation for regexec function, solving trac.560.
  Replaced U0 by E_L in testsuite
  Replaced U0 by E_L in PyNEST examples
  Replaced U0 by E_L in SLI examples
  Code formatting
  Replacing internal C++ variable U0_ by E_L.

Conflicts:
	pynest/nest/tests/test_parrot_neuron_ps.py
@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Apr 18, 2016

Contributor

👍 from my side.

Contributor

heplesser commented Apr 18, 2016

👍 from my side.

@jougs

This comment has been minimized.

Show comment
Hide comment
@jougs

jougs Apr 21, 2016

Contributor

I gave up reviewing after I've read half of it and did not find a single error. So, pragmatic 👍 from my side as this is definitely better than what we had before. @tammoippen or @abigailm: if you agree with my lax style of reviewing, could you please merge?

Contributor

jougs commented Apr 21, 2016

I gave up reviewing after I've read half of it and did not find a single error. So, pragmatic 👍 from my side as this is definitely better than what we had before. @tammoippen or @abigailm: if you agree with my lax style of reviewing, could you please merge?

@tammoippen tammoippen merged commit f2067bc into nest:master Apr 22, 2016

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