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

Use updateValue< long > instead of update_value_int in quantal_stp_synapse #2307

Merged
merged 5 commits into from
Mar 27, 2022

Conversation

stinebuu
Copy link
Contributor

This PR fixes #2049

@clinssen clinssen requested review from clinssen and jougs March 7, 2022 10:11
@clinssen clinssen added I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: Normal Handle this with default priority T: Maintenance Work to keep up the quality of the code and documentation. labels Mar 7, 2022
@clinssen clinssen added this to PRs in progress in Kernel via automation Mar 7, 2022
@clinssen clinssen added this to To do in Models via automation Mar 7, 2022
Kernel automation moved this from PRs in progress to PRs approved Mar 7, 2022
Copy link
Contributor

@jougs jougs left a comment

Choose a reason for hiding this comment

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

:-)

sli/dictutils.h Outdated Show resolved Hide resolved
sli/dictutils.h Outdated Show resolved Hide resolved
sli/dictutils.h Outdated Show resolved Hide resolved
models/quantal_stp_synapse_impl.h Outdated Show resolved Hide resolved
Kernel automation moved this from PRs approved to PRs pending approval Mar 7, 2022
@stinebuu stinebuu changed the title Move and rename update_value_int from quantal_stp_synapse.cpp Use updateValue< long > instead of update_value_int in quantal_stp_synapse Mar 22, 2022
@stinebuu stinebuu requested a review from clinssen March 22, 2022 08:06
@jougs
Copy link
Contributor

jougs commented Mar 22, 2022

One question: Does updateValue< long >also have the property this code will take either an int or a double and convert it to an int that the removed function had according to it's Doxygen comment?

@stinebuu
Copy link
Contributor Author

@jougs Good point. An error is now thrown if we provide a double. It is quite easy to fix by adding a check in the function of getValue< long >, should I do that?

getValue< long >( const Token& t )
{
const IntegerDatum* id = dynamic_cast< const IntegerDatum* >( t.datum() );
if ( id == NULL )
{ // We have to create a Datum object to get the name...
IntegerDatum const d;
throw TypeMismatch( d.gettypename().toString(), t.datum()->gettypename().toString() );
}
return id->get();
}

@jougs
Copy link
Contributor

jougs commented Mar 23, 2022

I think it would be good if that functionality were there. Otherwise we might again run into trouble when setting data from NEST Desktop, see #2009 and #2024.

@clinssen
Copy link
Contributor

I don't understand why this automatic cast is necessary; it could even be dangerous due to loss of precision. The underlying data type in class quantal_stp_synapse is an int. The cast, if it should happen at all, should be on the Python side, and NEST should error out when passed a float/double.

The only actual danger is mixing an int and a long here.

@jougs
Copy link
Contributor

jougs commented Mar 23, 2022

Argh! You're right, @clinssen. I was thinking the wrong way around all the time. This function updates a C++ variable of type long from a dictionary and that should of course throw an error if the datum in the dictionary is of type double. Sorry for confusing you, @stinebuu, I think you have to revert your recent commits aaf2fdd and 13055f1 😿

@stinebuu
Copy link
Contributor Author

I will convert back to 0434725 😉

Kernel automation moved this from PRs pending approval to PRs approved Mar 26, 2022
@heplesser heplesser merged commit be526a6 into nest:master Mar 27, 2022
Kernel automation moved this from PRs approved to Done (PRs and issues) Mar 27, 2022
@clinssen clinssen moved this from To do to Done in Models Nov 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: Normal Handle this with default priority T: Maintenance Work to keep up the quality of the code and documentation.
Projects
Kernel
  
Done
Models
  
Done
Development

Successfully merging this pull request may close these issues.

Move update_value_int from quantal_stp_synapse to where updateValue is defined
4 participants