Debug/profile declared private or accessible by CMAKE?
cguzman95 opened this issue · 5 comments
Hi @mattldawson ,
With all the new components in the GPU side (ODE solver, Linear solver etc) I'm adding a lot of code to debug/print stats(time execution/number of iterations...). I'm setting all of these code under different DEBUG or PRINT flags for each case (maybe I have a DEBUG_ITSOLVER or a DEBUG_CVODE_GPU something like that), instead of mixing all in the DEBUG flag and printing so much info that is hard to read (and also enabling the debug flag increase the total time execution and I want something more independent). So here is my question:
Where should I put this flags? Accessible throught the CMAKE or something private like a #define variable in the specific file that affects (e.g. DEBUG_ITSOLVER where be set in the file itsolver.cu).
In my opinion, it depends on the type of the DEBUG that we are doing:
- Profiling (time executions and iterations): Set as CMAKE variable. Maybe the user wants to check the performance of a component.
- Printing variables: Set as private variable in the file. Probably the user doesn't mind about this debug code, but for us developers these variables can help us when making changes.
- Checkings/testing of individual functions: Set as CMAKE variable?(doubting). Maybe the user wants to check if a specific function in the GPU (e,g. iterative solver) has some difference in accuracy infront their CPU alternative.
Let me know your opinion.
I like the idea about providing profiling information to the user if they want it. Eventually, I think we should remove most of the debugging code - once we're satisfied that everything is working, so I think doing a #define in the code is a good choice. But, I think having something like a ENABLE_PROFILIING flag might be useful. Maybe we could replace ENABLE_DEBUG with ENABLE_PROFILING eventually? I think we want to reduce the number of CMake flags because the number of possible combinations of flags is getting too large to make sure they all compile and run.
I agree we want to avoid printing things to the console - particularly when CAMP is running in a 3-D model like MONARCH. I would suggest collecting rather than printing this info when the user requests it.
Currently, when ENABLE_DEBUG is on, the user can a pass a solver_stats_t object to camp_core%run() that collects solver statistics (number of f(), Jac() calls, failure codes, etc.). In MONARCH we plot this during the run to see how the solver is running across all the grid cells. Does it make sense to include GPU profiling in a similar object? or maybe the same one? We could modify this object to include GPU profiling info and be enabled with ENABLE_PROFILING if necessary.
About change ENABLE_DEBUG with ENABLE_PROFILING: Maybe yes. After all solver_stats is mainly composed by time executions and number of iterations (e.g LS_setups, Jac_evals_total, Jac_time_s...). Maybe is more "appropiate" call it profiling.
I agree with reducing the number of CMake flags because can be a chaos have all this flags present... But I think we can solve this by dividing the CMake file into multiple CMakes (Like CVODE does). In this way the user will have only one ENABLE_PROFILING flag to enable all, but then when developing we only need to modfiy the specific private CMake file flag variable to print the specific interesting part we want.
In fact, this approach is similar to collecting all the data in solver_stats_t and printing them. The difference is that instead of merge all the stats in the same struct/class, now all the components/classes has his own flag to create, collect and print stats. In this way all the stats are independent and encapsullated and when developing we can easily avoid collecting data from classes than we don't need.
E.g. I only need to print GPU stats, so I left ENABLE_DEBUG OFF and only enabling a flag in the CMake GPU file I can collect this specific data without adding the noise from collecting stats of other parts of the code (This is very important because otherwise a noticeable part of the total time execution comes from collecting stats from all the code, when we only want from a small part).
that sounds good to me - maybe give it a try and we can review and revise as necessary?
Yes, I will reallocate my current DEBUG_GPU flag following this idea before the merge with chem_mod
Seems CMake definitions only affects the current directory, so I can't apply this definitions in a separate CMakeLists... well, at the moment I left it as a only option ENABLE_DEBUG and put rest of debug definitions alongside add_definitions(-DPMC_USE_GPU) whenENABLE_DEBUG is on