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

Improving the hit-time and cluster filter processors with proper input and output collections #19

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

angirar
Copy link

@angirar angirar commented Nov 12, 2024

No description provided.

@spagangriso
Copy link

Tagging @madbaron , @kkrizka ; this PR implements a couple of things, but mainly we have adapted the timing hit filter so that it works also in cases where 1 sim hit <-> 1 reco hit assumption is not true anymore.

I'm not 100% sure on the changes on the CMakeLists.txt file; I wonder if we have some good example we should follow for the current setup of packages.

@madbaron
Copy link

I don't think we have an example set up yet, but perhaps I can invoke @tmadlener and ask him to take a look specifically at CMakeLists.txt?

Copy link

@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.

You will probably have to rebase this onto the master branch after #20 (or merge that in). From a quick look, there is a chance for merge conflicts in the process. Let me know if you need help with that.

I have otherwise effectively only looked at the changes for the CMakeLists.txt

Comment on lines -23 to -38
# load default settings from ILCSOFT_CMAKE_MODULES
INCLUDE( ilcsoft_default_settings )

SET( COMPILER_FLAGS "-Wno-effc++" )
MESSAGE( STATUS "FLAGS ${COMPILER_FLAGS}" )
FOREACH( FLAG ${COMPILER_FLAGS} )
CHECK_CXX_COMPILER_FLAG( "${FLAG}" CXX_FLAG_WORKS_${FLAG} )
IF( ${CXX_FLAG_WORKS_${FLAG}} )
MESSAGE ( STATUS "Adding ${FLAG} to CXX_FLAGS" )
### We append the flag, so they overwrite things from somewhere else
SET ( CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${FLAG} ")
ELSE()
MESSAGE ( STATUS "NOT Adding ${FLAG} to CXX_FLAGS" )
ENDIF()
ENDFOREACH()

Choose a reason for hiding this comment

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

I would keep these. With #20 the warnings should be fixed so that output should be manageable again.

Comment on lines -131 to +114
# needed for adding header files to xcode project
IF(CMAKE_GENERATOR MATCHES "Xcode")
FILE( GLOB_RECURSE library_headers "*.h" )
ADD_SHARED_LIBRARY( ${PROJECT_NAME} ${library_sources} ${library_headers})
ELSE()
ADD_SHARED_LIBRARY( ${PROJECT_NAME} ${library_sources} )
ENDIF()

INSTALL_SHARED_LIBRARY( ${PROJECT_NAME} DESTINATION lib )

# display some variables and write them to cache
DISPLAY_STD_VARIABLES()
ADD_LIBRARY( ${PROJECT_NAME} SHARED ${library_sources} )

Choose a reason for hiding this comment

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

No strong opinion on removing this. It's still present in iLCSoft/MarlinTrkProcessors. However what definitely has to stay is the installation of the library that we build, as otherwise we will not be able to use the processors when we use this into a software stack.

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.

4 participants