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

Spike detector precision implicit setting and variable name #432

Closed
wants to merge 5 commits into from

Conversation

sepehrmn
Copy link
Contributor

@sepehrmn sepehrmn commented Jul 27, 2016

Two improvements:

  1. "user_set_precise_times" is currently used to store whether the precise_times property been directly set and "precision" of the recording_device the actual value. By changing to "has_user_set_precise_times", one could readily understand the point of this variable without wasting time.
  2. By setting "precision" one is implicitly setting the value of the variable above so in the set method it is necessary to set it to true if "precision" is directly set.

@sepehrmn sepehrmn changed the title Spike detector precision setting and variable name Spike detector precision implicit setting and variable name Jul 27, 2016
@sepehrmn
Copy link
Contributor Author

I suggest @abigailm as a reviewer.

@heplesser
Copy link
Contributor

This is an extended version of #431; I suggest to close that one.

@@ -45,7 +45,7 @@ nest::spike_detector::spike_detector()
: Node()
// record time and gid
, device_( *this, RecordingDevice::SPIKE_DETECTOR, "gdf", true, true )
, user_set_precise_times_( false )
, has_user_set_precise_times_( false )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't agree with this name change, I think the original name is sufficiently clear (note that "set" is past tense in the original name).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dear Hans-Ekkehard, the first time I saw it, I read "user_set" word as a compound adjective. I will revert this change.

@heplesser
Copy link
Contributor

@sepehrmn I don't understand what you want to achieve with your changes. Could you describe the use case you are trying to address?

@sepehrmn
Copy link
Contributor Author

sepehrmn commented Aug 2, 2016

There are two issue left, the discrepancy in setting the value of precision and the default value of "precision". In #431 I offered a solution for the latter, but as @heplesser pointed out, it was not a good one. I try to offer a better one with a new pull request.

If there are precise models, and a user sets "precise_times" to true, everything goes fine. But if there are precise models and it is not set, there can be a few problems:

  1. The precision value set by the user is ignored and it is set to 15. NEST does give the warning message below and it prints the value set, but it does not directly point to why the specified "precision" value was ignored and how one can set it to other than 15. "precision" is not exclusive to precise models. I will push a fix for this.

    Precise neuron models exist: the property precise_times of the
    spike_detector with gid 2 has been set to true, precision has been set to
    15.

  2. The default precision is 3 on recording_device, but if precise models exist and "precise times" is not set, spike_detector deviates from this and sets it to 15. A warning is printed, but would it not make sense for everything to be consistent and only have 1 default value to exist? It makes sense to use a higher precision for precise models as pointed out in Changed default "precision" value of recording_device #431 , so a new pull request I will submit will have value 3 as default if no precise models exist and 15 if they do and user has not set the precision directly.

@sepehrmn
Copy link
Contributor Author

sepehrmn commented Aug 3, 2016

This has morphed into #446 so I am closing it. Thanks @heplesser !

@sepehrmn sepehrmn closed this Aug 3, 2016
@sepehrmn sepehrmn deleted the spike_detector_varname branch November 16, 2016 04:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants