APRIL and SDHCALContent integration #29

tanguypasquier · 2024-07-10T15:11:33Z

BEGINRELEASENOTES

Add ability to link with new PFA contents : SDHCALContent for SDHCAL specific corrections and APRILContent for APRIL PFA algorithms.
Allow to compile with or without linking to new PFA Contents, default is to build without new contents.
Add processor option to switch between Pandora and APRIL PFAs.
ENDRELEASENOTES

tmadlener

Sorry for the long delay, this slipped through during summer / vacation time. Overall this looks pretty good to me. I think we should also try and get APRILContent and SDHCalContent into the Key4hep nightly builds to iron out all potential dependency issues and to also enable at least building these options in CI.

I have effectively mainly a few minor comments for the cmake configuration and some code formatting. The biggest thing I would change is the implementation of the usage of the hit factory that currently initializes creates two factories, when only one would be necessary.

tmadlener · 2024-08-28T12:30:43Z

CMakeLists.txt

+    IF( APRILContent_FOUND )
+        INCLUDE_DIRECTORIES( SYSTEM ${APRILContent_INCLUDE_DIRS} )
+        LINK_LIBRARIES( ${APRILContent_LIBRARIES} )
+        ADD_DEFINITIONS ( ${APRILContent_DEFINITIONS} )
+        ADD_DEFINITIONS( "-DAPRILCONTENT" )
+    ENDIF()
+    IF( mlpack_FOUND )
+        INCLUDE_DIRECTORIES( SYSTEM ${mlpack_INCLUDE_DIRS} )
+        LINK_LIBRARIES( ${mlpack_LIBRARIES} )
+        ADD_DEFINITIONS ( ${mlpack_DEFINITIONS} )
+    ENDIF()


Suggested change

IF( APRILContent_FOUND )

INCLUDE_DIRECTORIES( SYSTEM ${APRILContent_INCLUDE_DIRS} )

LINK_LIBRARIES( ${APRILContent_LIBRARIES} )

ADD_DEFINITIONS ( ${APRILContent_DEFINITIONS} )

ADD_DEFINITIONS( "-DAPRILCONTENT" )

ENDIF()

IF( mlpack_FOUND )

INCLUDE_DIRECTORIES( SYSTEM ${mlpack_INCLUDE_DIRS} )

LINK_LIBRARIES( ${mlpack_LIBRARIES} )

ADD_DEFINITIONS ( ${mlpack_DEFINITIONS} )

ENDIF()

INCLUDE_DIRECTORIES( SYSTEM ${APRILContent_INCLUDE_DIRS} )

LINK_LIBRARIES( ${APRILContent_LIBRARIES} )

ADD_DEFINITIONS ( ${APRILContent_DEFINITIONS} )

ADD_DEFINITIONS( "-DAPRILCONTENT" )

INCLUDE_DIRECTORIES( SYSTEM ${mlpack_INCLUDE_DIRS} )

LINK_LIBRARIES( ${mlpack_LIBRARIES} )

ADD_DEFINITIONS ( ${mlpack_DEFINITIONS} )

If you have REQUIRED in your find_package call there is no need to check again if it is actually found, because CMake will actually stop if it is not found.

Suggested change

IF( APRILContent_FOUND )

INCLUDE_DIRECTORIES( SYSTEM ${APRILContent_INCLUDE_DIRS} )

LINK_LIBRARIES( ${APRILContent_LIBRARIES} )

ADD_DEFINITIONS ( ${APRILContent_DEFINITIONS} )

ADD_DEFINITIONS( "-DAPRILCONTENT" )

ENDIF()

IF( mlpack_FOUND )

INCLUDE_DIRECTORIES( SYSTEM ${mlpack_INCLUDE_DIRS} )

LINK_LIBRARIES( ${mlpack_LIBRARIES} )

ADD_DEFINITIONS ( ${mlpack_DEFINITIONS} )

ENDIF()

IF( APRILContent_FOUND )

INCLUDE_DIRECTORIES( SYSTEM ${APRILContent_INCLUDE_DIRS} )

LINK_LIBRARIES( ${APRILContent_LIBRARIES} )

ADD_DEFINITIONS ( ${APRILContent_DEFINITIONS} )

ADD_DEFINITIONS( "APRILCONTENT" )

ENDIF()

IF( mlpack_FOUND )

INCLUDE_DIRECTORIES( SYSTEM ${mlpack_INCLUDE_DIRS} )

LINK_LIBRARIES( ${mlpack_LIBRARIES} )

ADD_DEFINITIONS ( ${mlpack_DEFINITIONS} )

ENDIF()

Deleted the IF(... FOUND) in the commit "CMakeLists updates" (d49c7a0)

I tried to delete the -D in the add definitions but it seems that the compiler wasn't understanding the definition as a macro anymore and was trying to find it as file or directory, thus creating errors during compilation

The special handling of adding -D might only be available for target_compile_definitions(?) @andresailer

tmadlener · 2024-08-28T12:32:04Z

CMakeLists.txt

+    IF( SDHCALContent_FOUND )
+        INCLUDE_DIRECTORIES( SYSTEM ${SDHCALContent_INCLUDE_DIRS} )
+        LINK_LIBRARIES( ${SDHCALContent_LIBRARIES} )
+        ADD_DEFINITIONS ( ${SDHCALContent_DEFINITIONS} )
+        ADD_DEFINITIONS( "-DSDHCALCONTENT" )
+    ENDIF()


Suggested change

IF( SDHCALContent_FOUND )

INCLUDE_DIRECTORIES( SYSTEM ${SDHCALContent_INCLUDE_DIRS} )

LINK_LIBRARIES( ${SDHCALContent_LIBRARIES} )

ADD_DEFINITIONS ( ${SDHCALContent_DEFINITIONS} )

ADD_DEFINITIONS( "-DSDHCALCONTENT" )

ENDIF()

INCLUDE_DIRECTORIES( SYSTEM ${SDHCALContent_INCLUDE_DIRS} )

LINK_LIBRARIES( ${SDHCALContent_LIBRARIES} )

ADD_DEFINITIONS ( ${SDHCALContent_DEFINITIONS} )

ADD_DEFINITIONS( "-DSDHCALCONTENT" )

If you have REQUIRED in your find_package call there is no need to check again if it is actually found, because CMake will actually stop if it is not found.

Suggested change

IF( SDHCALContent_FOUND )

INCLUDE_DIRECTORIES( SYSTEM ${SDHCALContent_INCLUDE_DIRS} )

LINK_LIBRARIES( ${SDHCALContent_LIBRARIES} )

ADD_DEFINITIONS ( ${SDHCALContent_DEFINITIONS} )

ADD_DEFINITIONS( "-DSDHCALCONTENT" )

ENDIF()

IF( SDHCALContent_FOUND )

INCLUDE_DIRECTORIES( SYSTEM ${SDHCALContent_INCLUDE_DIRS} )

LINK_LIBRARIES( ${SDHCALContent_LIBRARIES} )

ADD_DEFINITIONS ( ${SDHCALContent_DEFINITIONS} )

ADD_DEFINITIONS( "SDHCALCONTENT" )

ENDIF()

cmake will add -D when needed, and in places where we have to treat definitions programatically it is easier if we don't have to deal with some with -D and some without.

Same as above

tmadlener · 2024-08-28T12:36:33Z

cmake/Findmlpack.cmake

+FIND_PACKAGE_HANDLE_STANDARD_ARGS(mlpack DEFAULT_MSG mlpack_CORE_HPP_DIR)
+
+IF(NOT mlpack_FOUND)
+  MESSAGE(FATAL_ERROR "The mlpack package not found.")
+ENDIF()


Suggested change

FIND_PACKAGE_HANDLE_STANDARD_ARGS(mlpack DEFAULT_MSG mlpack_CORE_HPP_DIR)

IF(NOT mlpack_FOUND)

MESSAGE(FATAL_ERROR "The mlpack package not found.")

ENDIF()

include(FindPackageHandleStandardArgs)

FIND_PACKAGE_HANDLE_STANDARD_ARGS(mlpack DEFAULT_MSG mlpack_CORE_HPP_DIR)

Make sure that FIND_PACKAGE_HANDLE_STANDARD_ARGS is actually always available

FIND_PACKAGE_HANDLE_STANDARD_ARGS already does the correct handling of REQUIRED, QUIET, ... (the arguments that are passed to find_package).

Fixed in the "Fixed Findmlpack.cmake" (d8f1d2c)

cmake/Findmlpack.cmake

tmadlener · 2024-08-28T12:52:16Z

include/DDPandoraPFANewProcessor.h

+/**
+*  @brief  Register the LCContent plugins outside their original function. Temporary fix. Would be better to change the plugins registration directly in LCContent
+*/
+pandora::StatusCode PandoraHack(const pandora::Pandora &pandora);


Suggested change

pandora::StatusCode PandoraHack(const pandora::Pandora &pandora);

pandora::StatusCode AprilLCContentPluginRegistration(const pandora::Pandora &pandora);

(or a more fitting name)

Would at least hide a bit that this is a "hack", while also making it rather clear from the function name what is happening (on a high level).

I updated the name in the commit "Fixed the factory issue" (d793e70)

tmadlener · 2024-08-28T12:53:56Z

src/DDCaloHitCreator.cc

@@ -70,6 +80,7 @@ DDCaloHitCreator::~DDCaloHitCreator()

 pandora::StatusCode DDCaloHitCreator::CreateCaloHits(const EVENT::LCEvent *const pLCEvent)
 {
+    ChooseFactory(); //New method to choose the factory to use


Does this have to happen every event? This could be done only once in init where all the necessary information should also be available, right?

Indeed, I moved it to the DDCaloHitCreator constructor so it's now called only once in the init

tmadlener · 2024-08-28T12:56:51Z

include/DDCaloHitCreator.h

+#endif
+
+    //Added by T.Pasquier
+    pandora::ObjectFactory<object_creation::CaloHit::Parameters, object_creation::CaloHit::Object>* caloHitFactory; //General factory to initialize


Suggested change

pandora::ObjectFactory<object_creation::CaloHit::Parameters, object_creation::CaloHit::Object>* caloHitFactory; //General factory to initialize

pandora::ObjectFactory<object_creation::CaloHit::Parameters, object_creation::CaloHit::Object>* m_caloHitFactory; //General factory to initialize

To make it easier to spot that this is a member variable. (Requires some changes in the .cc file as well)

Given the overall usage pattern in the code, I would actually make this

std::unique_ptr<pandora::ObjectFactory<object_creation::CaloHit::Parameters, object_creation::CaloHit::Object>> m_caloHitFactory{nullptr};

(#include <memory> for unique_ptr)

Then ChooseFactory would become something like

#ifdef APRILCONTENT if(m_settings.m_useAPRIL) { caloHitFactory = std::make_unique<april_content::CaloHitFactory>(); } else #endif { caloHitFactory = std::make_unique<pandora::PandoraObjectFactory<object_creation::CaloHit::Parameters, object_creation::CaloHit::Object>>(); }

And the DefaultCaloFactory as well as the m_pAPRILCaloHitFactory member variables could go.

Usage in the implementation would then be

PANDORA_THROW_RESULT_IF(pandora::STATUS_CODE_SUCCESS, !=, PandoraApi::CaloHit::Create(m_pandora, caloHitParameters, *m_caloHitFactory));

Thank you very much! I fixed it and it works perfectly

src/DDPandoraPFANewProcessor.cc

andresailer · 2024-08-28T13:46:08Z

cmake/Findmlpack.cmake

mlpack should come with a cmake config file, so this findmlpack should not be needed
https://github.com/mlpack/mlpack/blob/master/CMake/mlpack-config.cmake.in

@tanguypasquier did you check, whether you can simply remove this file and things keep working?

I indeed tried to simply remove the file but it generates some errors during the compilation. I get :

`By not providing "Findmlpack.cmake" in CMAKE_MODULE_PATH this project has
asked CMake to find a package configuration file provided by "mlpack", but
CMake did not find one.

Could not find a package configuration file provided by "mlpack" with any
of the following names:

mlpackConfig.cmake
mlpack-config.cmake

Add the installation prefix of "mlpack" to CMAKE_PREFIX_PATH or set
"mlpack_DIR" to a directory containing one of the above files. If "mlpack"
provides a separate development package or SDK, be sure it has been installed.`

I checked in the mlpack files for the ones mentioned in the CMake error but couldn't find any of them.

I had already encountered this error during the APRIL installation and I then came across this issue (mlpack/mlpack#444) which seemed to indicate that the best option was to have the Findmlpack.cmake file.
As it solved the problem, I didn't really tried other ways of doing it since then

Where are you getting your mlpack from? It looks like it should be shipping a cmake configuration since v3.2.2 in principle. It might just be a question of pointing cmake into the right direction.

Sorry for the long delay, I had some other projects going on

I'm getting mlpack from their github repo (https://github.com/mlpack/mlpack) and I'm currently working with the v.4.0.0.
I tried a lot of different things in the last few weeks to make it work without the Findmlpack but didn't succeed..

Seems that the creating of the mlpack-config file got lost during their refactoring :/
They do write a pkgconfig file that one can also use via cmake, if PkgConfig is available.

Thanks a lot for the quick reply!

I succeeded in using PkgConfig via cmake to find mlpack without needing the Findmlpack file.
I will update the README with the new instructions and push this new version right away.

andresailer · 2024-08-28T13:46:58Z

CMakeLists.txt

+    IF( SDHCALContent_FOUND )
+        INCLUDE_DIRECTORIES( SYSTEM ${SDHCALContent_INCLUDE_DIRS} )
+        LINK_LIBRARIES( ${SDHCALContent_LIBRARIES} )
+        ADD_DEFINITIONS ( ${SDHCALContent_DEFINITIONS} )
+        ADD_DEFINITIONS( "-DSDHCALCONTENT" )
+    ENDIF()


Suggested change

IF( SDHCALContent_FOUND )

INCLUDE_DIRECTORIES( SYSTEM ${SDHCALContent_INCLUDE_DIRS} )

LINK_LIBRARIES( ${SDHCALContent_LIBRARIES} )

ADD_DEFINITIONS ( ${SDHCALContent_DEFINITIONS} )

ADD_DEFINITIONS( "-DSDHCALCONTENT" )

ENDIF()

IF( SDHCALContent_FOUND )

INCLUDE_DIRECTORIES( SYSTEM ${SDHCALContent_INCLUDE_DIRS} )

LINK_LIBRARIES( ${SDHCALContent_LIBRARIES} )

ADD_DEFINITIONS ( ${SDHCALContent_DEFINITIONS} )

ADD_DEFINITIONS( "SDHCALCONTENT" )

ENDIF()

cmake will add -D when needed, and in places where we have to treat definitions programatically it is easier if we don't have to deal with some with -D and some without.

andresailer · 2024-08-28T13:47:41Z

CMakeLists.txt

+    IF( APRILContent_FOUND )
+        INCLUDE_DIRECTORIES( SYSTEM ${APRILContent_INCLUDE_DIRS} )
+        LINK_LIBRARIES( ${APRILContent_LIBRARIES} )
+        ADD_DEFINITIONS ( ${APRILContent_DEFINITIONS} )
+        ADD_DEFINITIONS( "-DAPRILCONTENT" )
+    ENDIF()
+    IF( mlpack_FOUND )
+        INCLUDE_DIRECTORIES( SYSTEM ${mlpack_INCLUDE_DIRS} )
+        LINK_LIBRARIES( ${mlpack_LIBRARIES} )
+        ADD_DEFINITIONS ( ${mlpack_DEFINITIONS} )
+    ENDIF()


Suggested change

IF( APRILContent_FOUND )

INCLUDE_DIRECTORIES( SYSTEM ${APRILContent_INCLUDE_DIRS} )

LINK_LIBRARIES( ${APRILContent_LIBRARIES} )

ADD_DEFINITIONS ( ${APRILContent_DEFINITIONS} )

ADD_DEFINITIONS( "-DAPRILCONTENT" )

ENDIF()

IF( mlpack_FOUND )

INCLUDE_DIRECTORIES( SYSTEM ${mlpack_INCLUDE_DIRS} )

LINK_LIBRARIES( ${mlpack_LIBRARIES} )

ADD_DEFINITIONS ( ${mlpack_DEFINITIONS} )

ENDIF()

IF( APRILContent_FOUND )

INCLUDE_DIRECTORIES( SYSTEM ${APRILContent_INCLUDE_DIRS} )

LINK_LIBRARIES( ${APRILContent_LIBRARIES} )

ADD_DEFINITIONS ( ${APRILContent_DEFINITIONS} )

ADD_DEFINITIONS( "APRILCONTENT" )

ENDIF()

IF( mlpack_FOUND )

INCLUDE_DIRECTORIES( SYSTEM ${mlpack_INCLUDE_DIRS} )

LINK_LIBRARIES( ${mlpack_LIBRARIES} )

ADD_DEFINITIONS ( ${mlpack_DEFINITIONS} )

ENDIF()

tmadlener · 2024-08-28T15:26:22Z

I think you will also have to rebase this onto the latest version of iLCSoft/DDMarlinPandora, as it looks like there is a merge conflict (which may resolve itself automatically during rebasing). Additionally, that will pick up the latest configuration for the CI.

tmadlener · 2024-09-23T13:15:26Z

cmake/Findmlpack.cmake

@tanguypasquier did you check, whether you can simply remove this file and things keep working?

tmadlener · 2024-09-23T13:18:11Z

CMakeLists.txt

+    IF( APRILContent_FOUND )
+        INCLUDE_DIRECTORIES( SYSTEM ${APRILContent_INCLUDE_DIRS} )
+        LINK_LIBRARIES( ${APRILContent_LIBRARIES} )
+        ADD_DEFINITIONS ( ${APRILContent_DEFINITIONS} )
+        ADD_DEFINITIONS( "-DAPRILCONTENT" )
+    ENDIF()
+    IF( mlpack_FOUND )
+        INCLUDE_DIRECTORIES( SYSTEM ${mlpack_INCLUDE_DIRS} )
+        LINK_LIBRARIES( ${mlpack_LIBRARIES} )
+        ADD_DEFINITIONS ( ${mlpack_DEFINITIONS} )
+    ENDIF()


The special handling of adding -D might only be available for target_compile_definitions(?) @andresailer

tmadlener · 2024-09-23T13:19:50Z

CMakeLists.txt

+    FIND_PACKAGE( SDHCALContent REQUIRED )
+    INCLUDE_DIRECTORIES( SYSTEM ${SDHCALContent_INCLUDE_DIRS} )
+    LINK_LIBRARIES( ${SDHCALContent_LIBRARIES} )
+    ADD_DEFINITIONS ( ${SDHCALContent_DEFINITIONS} )


SDHCALContent doesn't seem to export any definitions(?): https://github.com/tanguypasquier/SDHCALContent/blob/main/cmake/SDHCALContentConfig.cmake.in

Suggested change

ADD_DEFINITIONS ( ${SDHCALContent_DEFINITIONS} )

Indeed, I deleted it in one of the last commits.

tmadlener · 2024-09-23T13:20:42Z

CMakeLists.txt

+    FIND_PACKAGE( mlpack REQUIRED )
+    INCLUDE_DIRECTORIES( SYSTEM ${APRILContent_INCLUDE_DIRS} )
+    LINK_LIBRARIES( ${APRILContent_LIBRARIES} )
+    ADD_DEFINITIONS ( ${APRILContent_DEFINITIONS} )


Suggested change

ADD_DEFINITIONS ( ${APRILContent_DEFINITIONS} )

APRILContent doesn't seem to export any definitions(?): https://github.com/SDHCAL/APRILContent/blob/current/cmake/APRILContentConfig.cmake.in

Same as for SDHCALContent, I also deleted it

tmadlener · 2024-09-23T13:27:24Z

include/DDCaloHitCreator.h

+    /* DDCaloHitCreator& operator=(const DDCaloHitCreator&) = delete;  // Disallow copying
+    DDCaloHitCreator(const DDCaloHitCreator&) = delete; */


Suggested change

/* DDCaloHitCreator& operator=(const DDCaloHitCreator&) = delete; // Disallow copying

DDCaloHitCreator(const DDCaloHitCreator&) = delete; */

public:

DDCaloHitCreator& operator=(const DDCaloHitCreator&) = delete;

DDCaloHitCreator(const DDCaloHitCreator&) = delete;

Since you know have a unique_ptr member copying won't work in any case. Marking these as = delete will improve the compiler error message. (I would move them to the rest of the constructors a bit further above).

I uncommented it and moved it as suggested!

…tent during compilation

…ation OK. 3 over 4 crash at runtime : commented out until cleaning in APRIL is done

…er 4 APRIL Registration OK

…ectly in the Marlin call without having to compile. Modified the README

…ts depending on the PFA that is used

…d particleId plugins from LCContent when using APRIL

…ndmlpack file. Modified the README according to the new CMake command

tmadlener reviewed Aug 28, 2024

View reviewed changes

andresailer reviewed Aug 28, 2024

View reviewed changes

andresailer mentioned this pull request Aug 28, 2024

Add possibility to include SDHCalContent and AprilContent #26

Closed

tanguypasquier force-pushed the master branch from c37d689 to 4e33c8c Compare September 20, 2024 09:05

tmadlener reviewed Sep 23, 2024

View reviewed changes

tanguypasquier and others added 23 commits November 18, 2024 13:35

made the changes to allow the inclusion of SDHCALContent and APRILCon…

773120c

…tent during compilation

Update README.md

a1688f8

Update compilation instruction

2cb25d4

First attempt to really include APRILContent : 1 over 4 APRIL registr…

fef1c4e

…ation OK. 3 over 4 crash at runtime : commented out until cleaning in APRIL is done

Cleaned APRIL and solved the problem for the RegisterAlgorithms. 2 ov…

a37b650

…er 4 APRIL Registration OK

Added a parameter that allows to choose between Pandora and APRIL dir…

8113fd5

…ectly in the Marlin call without having to compile. Modified the README

Modified README and cleaned some comments

a082d62

Fixed the CaloHitFactory problem. Not fully optimized but works

18932b5

Created a new method that calls the right method to create the caloHi…

660b66d

…ts depending on the PFA that is used

Cleaned some comments

5bd5e1d

Fixed an issue with the Non-APRIL mode. Now works in all cases

03bcd53

Deleted useless include

5bf1d8e

Included the choice of the factory instead of the last option

889920b

Created a function allowing the registration of energy corrections an…

903b95d

…d particleId plugins from LCContent when using APRIL

Removed unused include

2275b71

Added some comments and modified README

e4b5239

Update README.md

9d8f1fc

Update README.md

8143413

CMakeLists updates

4ac6bef

Fixed Findmlpack.cmake

5fdb866

Fixed the factory issue

f79e346

Deleted the useless definitions in CMakeLists

5ef82f8

Uncommented the = delete in DDCaloHitCreator

002ec22

tanguypasquier force-pushed the master branch from b8fea1a to 002ec22 Compare November 18, 2024 12:45

Modified the CMakeLists to find mlpack with pkgconfig. Deleted the Fi…

147980f

…ndmlpack file. Modified the README according to the new CMake command

	pandora::StatusCode PandoraHack(const pandora::Pandora &pandora);
	pandora::StatusCode AprilLCContentPluginRegistration(const pandora::Pandora &pandora);

	pandora::ObjectFactory<object_creation::CaloHit::Parameters, object_creation::CaloHit::Object>* caloHitFactory; //General factory to initialize
	pandora::ObjectFactory<object_creation::CaloHit::Parameters, object_creation::CaloHit::Object>* m_caloHitFactory; //General factory to initialize

		/* DDCaloHitCreator& operator=(const DDCaloHitCreator&) = delete; // Disallow copying
		DDCaloHitCreator(const DDCaloHitCreator&) = delete; */

APRIL and SDHCALContent integration #29

Are you sure you want to change the base?

APRIL and SDHCALContent integration #29

Conversation

tanguypasquier commented Jul 10, 2024

tmadlener left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmadlener commented Aug 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment