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

add double values to LCParameters #143

Merged
merged 6 commits into from
Nov 5, 2021
Merged

Conversation

gaede
Copy link
Contributor

@gaede gaede commented Oct 27, 2021

BEGINRELEASENOTES

  • add support for storing double values in LCParameters
    • used in run, event and collection parameters
    • example
	DoubleVec dv ;
	dv.push_back( 1.111111111111111111111111111111111111111111111111 ) ;
	dv.push_back( 2.222222222222222222222222222222222222222222222222 ) ;
	dv.push_back( 3.333333333333333333333333333333333333333333333333 ) ;
	evt->parameters().setValues( "SomeDoubleNumbers" , dv ) ;

ENDRELEASENOTES

Copy link
Contributor

@tmadlener tmadlener left a comment

Choose a reason for hiding this comment

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

Looks good to me. I just have a question about the versioning and how it relates to I/O and some potential improvements (which would probably also apply to other parts of the LCParameters handling)

Comment on lines +43 to +48
int nDouble ;
SIO_DATA( device , &nDouble , 1 ) ;
EVENT::DoubleVec doubleVec(nDouble) ;
for(int j=0; j< nDouble ; j++ ) {
SIO_DATA( device , &doubleVec[j] , 1 ) ;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
int nDouble ;
SIO_DATA( device , &nDouble , 1 ) ;
EVENT::DoubleVec doubleVec(nDouble) ;
for(int j=0; j< nDouble ; j++ ) {
SIO_DATA( device , &doubleVec[j] , 1 ) ;
}
EVENT::DoubleVec doubleVec;
SIO_SDATA(device, doubleVec);

sio should be able to handle vectors as a whole, without having to read each element separately. We do this in podio: https://github.com/AIDASoft/podio/blob/master/src/SIOBlock.cc#L47-L67

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right of course. I have here intentionally used the same code structure as for the other parameters for 'consistency'. I believe we should create a separate PR and apply this optimization in all streaming handlers...

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed :) I will create an issue to keep track of it.

Comment on lines +104 to +107
SIO_DATA( device , &nDouble , 1 ) ;
for(int j=0; j< nDouble ; j++ ){
SIO_SDATA( device, doubleVec[j] ) ;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
SIO_DATA( device , &nDouble , 1 ) ;
for(int j=0; j< nDouble ; j++ ){
SIO_SDATA( device, doubleVec[j] ) ;
}
SIO_SDATA(device, doubleVec);

See above.

CMakeLists.txt Show resolved Hide resolved
@tmadlener tmadlener changed the title WPI: add double values to LCParameters add double values to LCParameters Nov 5, 2021
@tmadlener tmadlener merged commit bc119cd into iLCSoft:master Nov 5, 2021
@tmadlener tmadlener mentioned this pull request Nov 7, 2022
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.

Question/feature request: squared matrix element as double instead of float
3 participants