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

Spin_detector precision and documentation fix #767

Merged
merged 7 commits into from Jul 7, 2017

Conversation

@sepehrmn
Copy link
Contributor

sepehrmn commented Jun 25, 2017

Fix for the first issue of #698. Precision was already there for some current or future use so I made it compatible with #446.

I'll fix the documentation of the spin_detector in a separate pull request. It got me thinking, maybe it could be made compatible with more than two states; a model I'm working on can have 4 states.

@sepehrmn
Copy link
Contributor Author

sepehrmn commented Jun 26, 2017

This can now be merged. Should be straightforward.

@heplesser heplesser requested review from jougs and apeyser Jun 29, 2017
Copy link
Contributor

jougs left a comment

Many thanks for the PR. I will approve and merge it once my comments are addressed.

@@ -46,7 +46,9 @@ By default, GID and time of each spike is recorded.
The spike detector can also record spike times with full precision

This comment has been minimized.

@jougs

jougs Jun 29, 2017 Contributor

This should probably also be adapted to the spin detector, otherwise it is not addressing the first point of #698.

achieve this.
achieve this. If there are precise models and /precise_times is not
set, it will be set to True at the start of the simulation and
/precision will be increased to 15 from its default value of 3.
Any node from which spikes are to be recorded, must be connected to
the spike detector using a normal connect command. Any connection weight

This comment has been minimized.

@jougs

jougs Jun 29, 2017 Contributor

See above.


Second Version: June 2017
Description: added more tests
Author: Sepehr Mahmoudian

This comment has been minimized.

@jougs

jougs Jun 29, 2017 Contributor

A Second Version field is not recognized by the documentation parser, as isn't a second Author field. Please remove both. You can add your name to the first Author field if you wish to be recognized. Please also remove the Description field you have added. Description is for describing the model, not the changes.

See https://github.com/nest/nest-simulator/blob/master/doc/doc_header.txt for allowed fields in SLI docstrings.

@sepehrmn
Copy link
Contributor Author

sepehrmn commented Jun 29, 2017

@jougs Many thanks for your comments. I'll address the issues over the weekend.

sepehrmn added 2 commits Jul 2, 2017
@sepehrmn
Copy link
Contributor Author

sepehrmn commented Jul 2, 2017

@jougs I have addressed all issues and wrote full documentation for the device as it was copy pasted from another device. This can now be merged.

@sepehrmn sepehrmn changed the title Spin_detector precision Spin_detector precision and documentation fix Jul 2, 2017
@jougs
jougs approved these changes Jul 5, 2017
Copy link
Contributor

jougs left a comment

Many thanks for addressing my issues.

@jougs
Copy link
Contributor

jougs commented Jul 7, 2017

@apeyser: most of the functionality touched here will have to be re-implemented in NESTio. Can I just merge this now and we review it anew when merging the nestio branch back to master?

@apeyser
Copy link
Contributor

apeyser commented Jul 7, 2017

@jougs: sounds reasonable.

@jougs
Copy link
Contributor

jougs commented Jul 7, 2017

@apeyser: thanks. Merging.

@jougs jougs merged commit bb3cafe into nest:master Jul 7, 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

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