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.

Sign up for GitHub

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

Jump to bottom

Automatic conversion of integers to doubles when setting parameters #2024

Merged
merged 13 commits into from
Jul 2, 2021
Merged

Automatic conversion of integers to doubles when setting parameters #2024

merged 13 commits into from
Jul 2, 2021

Conversation

ackurth
Copy link
Contributor

@ackurth ackurth commented Apr 23, 2021

Fixes #2009.
The implementation silently converts integers to doubles (both for simple and vector data) where doubles are expected.
We removed the tests that became obsolete due to this new behavior.

ackurth and others added 4 commits April 23, 2021 15:28
@ackurth @jasperalbers
First try to cast integer to double
18197dd 
Co-authored-by: jasperalbers <jasperalbers45@gmail.com>
@ackurth @jasperalbers
Cast integers to doubles for simple and vector data
2a0dc5f 
Co-authored-by: jasperalbers <jasperalbers45@gmail.com>
@ackurth @jasperalbers
Remove tests that are obsolete due to new feature
48172cd 
Co-authored-by: jasperalbers <jasperalbers45@gmail.com>
@ackurth
Merge branch 'master' into cast_integers_to_doubles
0c109b0
@heplesser heplesser mentioned this pull request Apr 26, 2021
Closed
@ackurth ackurth marked this pull request as draft April 26, 2021 07:52
@heplesser
Copy link
Contributor

@ackurth Could this PR be move from Draft to Open?

@ackurth
Copy link
Contributor Author

ackurth commented May 27, 2021

@ackurth Could this PR be move from Draft to Open?

The PR as it is right now does address the issue of passing integers where doubles are expected in cases like {'V_m' : 1}.
However, if a double-vector is expected - e.g. {'extent' : [-1, 1]} the conversion does not work yet.
This especially means that the example from the issue is not addressed yet.
I reached out to @hakonsbm to resolve this.

@ackurth ackurth marked this pull request as ready for review June 25, 2021 13:14
@stinebuu stinebuu added this to PRs in progress in Kernel via automation Jul 1, 2021
@stinebuu stinebuu 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: Enhancement New functionality, model or documentation labels Jul 1, 2021
@stinebuu
Copy link
Contributor

stinebuu commented Jul 1, 2021

@ackurth There seems to be some formatting errors, could you fix this?

heplesser
heplesser requested changes Jul 1, 2021
View reviewed changes
Copy link
Contributor

@heplesser heplesser left a comment

Choose a reason for hiding this comment

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

Thanks! I included some comments in the code below.

sli/tokenarray.cc Outdated Show resolved Hide resolved
sli/tokenutils.cc Outdated Show resolved Hide resolved
sli/tokenutils.cc Outdated
@@ -97,13 +106,20 @@ template <>
float
getValue< float >( const Token& t )
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 think we use float anywhere in NEST, could you try to remove this function and see if anything breaks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed this function from the code and everything seems to work fine, see c21eef4.

sli/tokenutils.cc Outdated
DoubleDatum* dd = dynamic_cast< DoubleDatum* >( t.datum() );
if ( dd )
{
return ( float ) dd->get();
Copy link
Contributor

Choose a reason for hiding this comment

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

Turn into proper static cast if kept.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See above.

sli/tokenutils.cc Outdated
IntegerDatum* id = dynamic_cast< IntegerDatum* >( t.datum() );
if ( id )
{
return ( float ) id->get();
Copy link
Contributor

Choose a reason for hiding this comment

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

See above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See above.

sli/tokenutils.cc Outdated

// we have to create a Datum object to get the name...
DoubleDatum const d;
throw TypeMismatch( d.gettypename().toString(), t.datum()->gettypename().toString() );
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Se if setValue< float > below can be removed as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above.

Kernel automation moved this from PRs in progress to PRs pending approval Jul 1, 2021
stinebuu
stinebuu reviewed Jul 2, 2021
View reviewed changes
sli/tokenutils.cc Outdated Show resolved Hide resolved
ackurth and others added 4 commits July 2, 2021 09:41
@ackurth @heplesser
Update sli/tokenarray.cc
c294ccc 
Co-authored-by: Hans Ekkehard Plesser <hans.ekkehard.plesser@nmbu.no>
@ackurth @heplesser
Update sli/tokenutils.cc
324d6ad 
Co-authored-by: Hans Ekkehard Plesser <hans.ekkehard.plesser@nmbu.no>
@ackurth
Remove unused getValue, setValue for float
c21eef4
@ackurth
Update comments
b1a8100
stinebuu
stinebuu approved these changes Jul 2, 2021
View reviewed changes
Copy link
Contributor

@stinebuu stinebuu left a comment

Choose a reason for hiding this comment

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

Thanks!

Kernel automation moved this from PRs pending approval to PRs approved Jul 2, 2021
@heplesser heplesser changed the title Cast integers to doubles Automatic conversion of integers to doubles when setting parameters Jul 2, 2021
@heplesser heplesser merged commit 888287a into nest:master Jul 2, 2021
Kernel automation moved this from PRs approved to Done (PRs and issues) Jul 2, 2021
This was referenced Jul 15, 2021
Closed
Merged
@jougs jougs mentioned this pull request Jul 15, 2021
Merged
@jougs jougs mentioned this pull request Mar 23, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@heplesser heplesser heplesser approved these changes

@stinebuu stinebuu stinebuu approved these changes

Assignees
No one assigned
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: Enhancement New functionality, model or documentation
Projects
Kernel
Status: Done
Kernel
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow integer values where doubles are expected in NEST
4 participants
@ackurth @heplesser @stinebuu @hakonsbm