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

Replace NAN & std::isnan by numerics::nan & numerics::isnan #127

Merged
merged 4 commits into from
Nov 4, 2015

Conversation

tammoippen
Copy link
Contributor

In response to trac#961 and #123.
We use NAN as a default argument in connect functions for weight and delay, as one can use both default delay and default weight, default delay and set a weight, default weight and set a delay or set a weight and a delay. Checking for isnan seems problematic on K. Hence, I implemented them in the numerics namespace to have more control over them:

  • In configure tests we check for all variations of nan/std::nan, isnan/std::isnan.
  • Depending on what is available, use it in numerics.h/.cpp and prefer the c++ versions.
  • If non is available or if IS_K is defined, use a handwritten isnan, that compares byte-wise to the defined numerics::nan. See StackOverflow for this.

* In configure tests check for all variations of nan/std::nan, isnan/std::isnan
* Depending on what is available, use it in numerics.h/.cpp ; prefer c++ versions.
* If non is available, use own implementation.
* If IS_K is defined, a handwritten isnan will be used, that compares bytewise to the defined numerics::nan
@janhahne
Copy link
Contributor

@tammoippen I assume the

#if defined( IS_K )
#undef HAVE_STD_ISNAN
#undef HAVE_ISNAN
#endif 

will not be needed. When I applied the fix proprosed in #123 the connections were build correctly. This was confirmed by running your sli test script successfully and by running the reproducer from trac#961, which works fine as well. So I guess there is no need to force K to use the handwritten isnan

@heplesser
Copy link
Contributor

@tammoippen Good work! But can we trust the handwritten version under all circumstances? I am afraid that weird things may happen on weird systems, and we currently have no tests to check that numerics::nan and numerics::isnan() actually work consistently, i.e., that numerics::isnan(x) returns true if an only if x == numerics::nan. Not sure how to add such a test to our testsuite ...

@JanneM
Copy link
Contributor

JanneM commented Oct 29, 2015

When I first encountered this issue I spent some time trying to figure out how to solve this in the correct way. I went as far as emailing a friend that develops in C++ for a living. The consensus seems to be that short of requiring a sufficiently recent version of the C++ standard, there is no general solution that is guaranteed to work in all cases.

@heplesser
Copy link
Contributor

@JanneM I suspected as much. Therefore my proposal to use the NaN support from either cmath or math.h combined with a test that our isnan based on those really works. In that way, we will cover, as far as I can see, all systems that we currently work with and be alerted to problems when porting to systems without proper NaN support. That seems safer to me than solutions relying on specific bit patterns.

@janhahne
Copy link
Contributor

@tammoippen The K compiler confuses the new function

template < class T >
bool
isnan( T f )

with the C macro and stops with an "expected identifier" error. Therefore the function needs another name. @heplesser suggested is_nan( T f ) in #123. Another question: Shouldn't it be < typename T > instead of < class T >?

I compiled NEST on K with the fixes of this pull request combined with pull request #130 and this works fine (after changing the function name to is_nan). I tested the functionallity of the result one time with and one time without deleting the #undefs from my comment above. Both work fine on K.

@heplesser Wouldn't the test @tammoippen send me be sufficient to confirm that the employed isnan really works (at least in the context it is used for)?

% This tests individual setting of weight and delay on synapses.
% 
% Four connections will be created:
% * 1 -> 5  : use default delay and default weight
% * 2 -> 5  : use default weight; set delay
% * 3 -> 5  : use default delay; set weight
% * 4 -> 5  : set delay and set weight
% 
% Afterwards the settings will be checked. Sample output:
%    $ nest connection_test.sli
%    NEST v2.8.0-git (C) 2004 The NEST Initiative
%    default weight and delay connection:
%      default weight = 2.3 synapse weight = 2.3 : equal ? true
%      default delay  = 1.5 synapse delay  = 1.5 : equal ? true
%    default weight; delay = 15.5ms connection:
%      default weight = 2.3  synapse weight = 2.3  : equal ? true
%      expected delay = 15.5 synapse delay  = 15.5 : equal ? true
%    default delay ; weight = 23.4 connection:
%      default weight = 23.4 synapse weight = 23.4 : equal ? true
%      default delay  = 1.5  synapse delay  = 1.5  : equal ? true
%    delay = 15.5 ms ; weight = 23.4 connection:
%      default weight = 23.4 synapse weight = 23.4 : equal ? true
%      expected delay = 15.5 synapse delay  = 15.5 : equal ? true
% 
% When 8 true's appear at the end, everything is set correctly.

/static_synapse << /delay 1.5 /weight 2.3 >> SetDefaults

/static_synapse GetDefaults /defaults Set

/d 15.5 def
/w 23.4 def

/iaf_neuron 5 Create ;

% default weight and delay
[1] cvgidcollection [5] cvgidcollection /one_to_one << /model /static_synapse >> Connect

% default weight; delay = 15ms
[2] cvgidcollection [5] cvgidcollection /one_to_one << /model /static_synapse /delay d >> Connect

% default delay; weight = 23
[3] cvgidcollection [5] cvgidcollection /one_to_one << /model /static_synapse /weight w >> Connect

% delay = 15ms ; weight = 23
[4] cvgidcollection [5] cvgidcollection /one_to_one << /model /static_synapse /delay d /weight w >> Connect

<< /source [1] >> GetConnections GetSynapseStatus 0 get /syn Set
(default weight and delay connection: \n) =only
(  default weight = ) =only defaults /weight get =only ( synapse weight = ) =only syn /weight get =only 
    ( : equal ? ) =only defaults /weight get syn /weight get eq ==
(  default delay  = ) =only defaults /delay get =only  ( synapse delay  = ) =only syn /delay get =only 
    ( : equal ? ) =only defaults /delay get syn /delay get eq ==

<< /source [2] >> GetConnections GetSynapseStatus 0 get /syn Set
(default weight; delay = ) =only d =only (ms connection: \n) =only
(  default weight = ) =only defaults /weight get =only (  synapse weight = ) =only syn /weight get =only 
    (  : equal ? ) =only defaults /weight get syn /weight get eq ==
(  expected delay = ) =only d =only ( synapse delay  = ) =only syn /delay get =only 
    ( : equal ? ) =only d syn /delay get eq ==

<< /source [3] >> GetConnections GetSynapseStatus 0 get /syn Set
(default delay ; weight = ) =only w =only ( connection: \n) =only
(  default weight = ) =only w =only ( synapse weight = ) =only syn /weight get =only 
    ( : equal ? ) =only w syn /weight get eq ==
(  default delay  = ) =only defaults /delay get =only  (  synapse delay  = ) =only syn /delay get =only 
    (  : equal ? ) =only defaults /delay get syn /delay get eq ==

<< /source [4] >> GetConnections GetSynapseStatus 0 get /syn Set
(delay = ) =only d =only ( ms ; weight = ) =only w =only ( connection: \n) =only
(  default weight = ) =only w =only ( synapse weight = ) =only syn /weight get =only 
    ( : equal ? ) =only w syn /weight get eq ==
(  expected delay = ) =only d =only ( synapse delay  = ) =only syn /delay get =only 
    ( : equal ? ) =only d syn /delay get eq ==

@heplesser
Copy link
Contributor

@janhahne @tammoippen

  • The problem with mixing up isnan is, I suppose, due to the string substitution done on macros, before the compiler ever sees the code. So calling the function is_nan() is probably the best way around.
  • It should be typename T``,class T` in connection with template declarations is considered outdated.
  • I think Tammo's test is fine, but we should run a manual check that the test fails if is_nan() does not work correctly: Compile once with the function always returning true, once always returning false; In both cases, the test should fail.

@janhahne
Copy link
Contributor

@heplesser I have run the manual check:

template < typename T >
bool
is_nan( T f )
{
return true;
}

gives you

default weight and delay connection: 
  default weight = 2.3 synapse weight = 2.3 : equal ? true
  default delay  = 1.5 synapse delay  = 1.5 : equal ? true
default weight; delay = 15.5ms connection: 
  default weight = 2.3  synapse weight = 2.3  : equal ? true
  expected delay = 15.5 synapse delay  = 1.5 : equal ? false
default delay ; weight = 23.4 connection: 
  default weight = 23.4 synapse weight = 2.3 : equal ? false
  default delay  = 1.5  synapse delay  = 1.5  : equal ? true
delay = 15.5 ms ; weight = 23.4 connection: 
  default weight = 23.4 synapse weight = 2.3 : equal ? false
  expected delay = 15.5 synapse delay  = 1.5 : equal ? false

and

template < typename T >
bool
is_nan( T f )
{
return false;
}

results in

default weight and delay connection: 
  default weight = 2.3 synapse weight = nan : equal ? false
  default delay  = 1.5 synapse delay  = 0 : equal ? false
default weight; delay = 15.5ms connection: 
  default weight = 2.3  synapse weight = nan  : equal ? false
  expected delay = 15.5 synapse delay  = 15.5 : equal ? true
default delay ; weight = 23.4 connection: 
  default weight = 23.4 synapse weight = 23.4 : equal ? true
  default delay  = 1.5  synapse delay  = 1.5  : equal ? true
delay = 15.5 ms ; weight = 23.4 connection: 
  default weight = 23.4 synapse weight = 23.4 : equal ? true
  expected delay = 15.5 synapse delay  = 15.5 : equal ? true

@heplesser
Copy link
Contributor

👍 for merge.

@jougs
Copy link
Contributor

jougs commented Nov 4, 2015

Very nice work indeed. I'm all 👍 for merging. Many thanks for all your valuable contributions.

jougs added a commit that referenced this pull request Nov 4, 2015
Replace NAN and std::isnan by numerics::nan and numerics::isnan
@jougs jougs merged commit 349c728 into nest:master Nov 4, 2015
@janhahne janhahne mentioned this pull request Nov 4, 2015
seeholza added a commit to seeholza/nest-simulator that referenced this pull request Nov 4, 2015
* master:
  Use NewConnect instead of DivergentConnect and remove non-default weight in favor of higher rate.
  Fixed error message and reporting in case CSA is not available.
  Make wording more consistent with respect to active and passive speech.
  improvements as discussed in PR nest#127
  Added reference
  Updated CSA examples to new documentation guidelines.
  Replace NAN, std::isnan by numerics::nan, numerics::isnan
@tammoippen tammoippen deleted the alternative_NAN_trac.961 branch November 12, 2015 12:12
tammoippen added a commit to tammoippen/nest-simulator that referenced this pull request Jan 7, 2016
# Conflicts:
#	libnestutil/numerics.cpp
#	libnestutil/numerics.h
#	nestkernel/connector_model.h
#	nestkernel/connector_model_impl.h
@gewaltig gewaltig mentioned this pull request Jan 12, 2016
heplesser added a commit that referenced this pull request Aug 3, 2018
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.

5 participants