Restructuring #2

AdityaPandeyCN · 2025-07-23T15:12:25Z

This PR restructures the project directory following our ROOT project. The benchmark folder and other files will be updated continuously to make it a proper package.

Signed-off-by: AdityaPandeyCN <[email protected]> restructured the directory Signed-off-by: AdityaPandeyCN <[email protected]> changed folder names Signed-off-by: AdityaPandeyCN <[email protected]> restructuring the project Signed-off-by: AdityaPandeyCN <[email protected]>

Signed-off-by: AdityaPandeyCN <[email protected]>

vgvassilev · 2025-07-28T06:59:38Z

tools/checkindex.cxx

-#include "utils.h"
-
-#include "ramrecord.C"
+#include "../inc/ttree/Utils.h"


If the tools folder is where the executables are we need to find a better solution to this. The CMakeLists in tools should start depending on ttree and then it will need to automatically adjust the include paths so that you do not need to type ../inc.

vgvassilev · 2025-07-28T07:00:37Z

tools/samtoramntuple.cxx

@@ -1,8 +1,9 @@
 // samtoramntuple.C
 // SAM to RAM converteR

-#include "RAMntuplerecord.C"
-#include "RAMNTupleRecord.h"
+#include "../src/rntuple/RAMNTupleRecord.cxx"


Why we have to include the cxx file here?

Signed-off-by: AdityaPandeyCN <[email protected]>

vgvassilev · 2025-07-28T17:09:38Z

CMakeLists.txt


-add_library(ramtools_tree STATIC
+include_directories(${CMAKE_CURRENT_SOURCE_DIR}/inc/ttree)


We should probably use target_include_directories. include_directories is not recommended unless there is a good reason for it.

vgvassilev · 2025-07-28T17:10:52Z

CMakeLists.txt

+ROOT_GENERATE_DICTIONARY(G__ramtools_tree
+    RAMRecord.h
+    Utils.h
+    CigarOps.h
+    LINKDEF ${CMAKE_CURRENT_SOURCE_DIR}/inc/ttree/LinkDef.h
+    MODULE ramtools_tree
+)
+
+add_library(ramtools_tree SHARED


Why not combining these two by using ROOT_STANDARD_LIBRARY_PACKAGE?

Getting errors after implementing this
CMake Error at /home/aditya/root-6.35-ntuple/share/root/cmake/RootMacros.cmake:1237 (install): install DIRECTORY given no DESTINATION! Call Stack (most recent call first): /home/aditya/root-6.35-ntuple/share/root/cmake/RootMacros.cmake:1428 (ROOT_INSTALL_HEADERS) CMakeLists.txt:22 (ROOT_STANDARD_LIBRARY_PACKAGE)

The message says it all, right? We need to pass some install directory of the component..

vgvassilev · 2025-07-28T17:11:49Z

tools/ramview.cxx

@@ -15,8 +15,8 @@
 #include <TTreeIndex.h>
 #include <TTreePerfStats.h>

-#include "../inc/ttree/Utils.h"
-#include "../inc/ttree/RAMRecord.h"
+#include "Utils.h"


Ideally these should be ttree/Utils.h to be able to know what's where...

Signed-off-by: AdityaPandeyCN <[email protected]>

CMakeLists.txt

vgvassilev · 2025-07-29T18:58:52Z

inc/rntuple/RAMNTupleRecord.h

+};
+
+// CIGAR operation codes (from BAM format)
+#include "../ttree/CigarOps.h"


Suggested change

#include "../ttree/CigarOps.h"

#include "ttree/CigarOps.h"

vgvassilev · 2025-07-29T19:00:02Z

inc/ttree/RAMRecord.h

-const UChar_t RAM_CIGAR_P     = 6;
-const UChar_t RAM_CIGAR_EQUAL = 7;
-const UChar_t RAM_CIGAR_X     = 8;
+#include "CigarOps.h"


Suggested change

#include "CigarOps.h"

#include "ttree/CigarOps.h"

vgvassilev · 2025-07-29T19:00:50Z

src/rntuple/RAMNTupleRecord.cxx

@@ -0,0 +1,749 @@
+//
+// RAMNTupleRecord.cpp


Suggested change

// RAMNTupleRecord.cpp

// RAMNTupleRecord.cxx

vgvassilev · 2025-07-29T19:02:07Z

src/rntuple/RAMNTupleRecord.cxx

+         const auto &entries = index_view(0);
+         fgIndex->SetEntries(entries);
+      } catch (...) {
+         // Field doesn't exist


That's something we should rework maybe not part of this PR. We should not rely on exceptions to check if a field exists or not...

vgvassilev · 2025-07-29T19:03:52Z

test/CMakeLists.txt

@@ -0,0 +1,37 @@
+include(FetchContent)


This is how we are doing this reliably: https://github.com/vgvassilev/clad/blob/master/cmake/modules/CladGoogleTest.cmake

vgvassilev · 2025-07-29T19:07:01Z

tools/CMakeLists.txt

+    if(${tool_name} MATCHES "ntuple")
+        target_link_libraries(${tool_name} PRIVATE
+            ramtools_ntuple
+            ROOT::Core
+            ROOT::RIO
+            ROOT::ROOTNTuple
+            ROOT::ROOTNTupleUtil
+        )
+    else()
+        target_link_libraries(${tool_name} PRIVATE
+            ramtools_tree
+            ROOT::Core
+            ROOT::RIO
+            ROOT::Tree
+        )


The dependencies should come from the outside as LIBRARY or something like that. We can use ROOT_EXECUTABLE.

Signed-off-by: AdityaPandeyCN <[email protected]>

AdityaPandeyCN · 2025-07-30T05:32:13Z

I have tried to address the suggestions also apologies for the foolish errors I have made.

vgvassilev

Can we add some basic linux CI?

.clang-format

vgvassilev · 2025-07-30T06:59:25Z

CMakeLists.txt

@@ -1,44 +1,75 @@
 cmake_minimum_required(VERSION 3.16)
+project(ramtools VERSION 1.0.0 LANGUAGES C CXX)  


Why C here?

Actually build was failing with empty CMAKE_C_COMPILER variable error so added C to check, now have removed passing it explicitly.

CMakeLists.txt

vgvassilev · 2025-07-30T07:00:31Z

CMakeLists.txt

-  SOURCES  ramrecord.C
-  HEADERS  ramrecord.h
-  LIBRARIES ROOT::Core ROOT::RIO ROOT::Tree
+include_directories(${CMAKE_CURRENT_SOURCE_DIR}/inc)


Doesn't ROOT_STANDARD_LIBRARY_PACKAGE set this properly up?

vgvassilev · 2025-07-30T07:00:53Z

CMakeLists.txt

+    INSTALL_OPTIONS
+        DESTINATION ${CMAKE_INSTALL_LIBDIR}
+)
+add_library(ramtools_ntuple SHARED


Why this is a separate library?

I thought it was better to separate them but now will make it one.

vgvassilev · 2025-07-30T07:01:51Z

CMakeLists.txt

 )

-ROOT_EXECUTABLE(samtoramntuple


Why that did not work?

Re-used this in the tools cmake, there was no reason I was just trying to use other ways

Please use the ROOT_EXECUTABLE where possible.

ping about this one.

vgvassilev · 2025-07-30T07:02:59Z

cmake/ramtoolsConfig.cmake.in

+@PACKAGE_INIT@
+
+include("${CMAKE_CURRENT_LIST_DIR}/ramtoolsTargets.cmake") 
+


Why is this file needed?

vgvassilev · 2025-07-30T07:04:22Z

test/CMakeLists.txt

+add_executable(CountEntriesTest CountEntriesTest.cxx)
+target_link_libraries(CountEntriesTest PRIVATE
+    ramtools_tree
+    ramtools_ntuple
+    gtest
+    gtest_main
+)
+add_test(NAME CountEntriesTest COMMAND CountEntriesTest)
+set_tests_properties(CountEntriesTest PROPERTIES
+    WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/tools
+    ENVIRONMENT "ROOT_INCLUDE_PATH=${CMAKE_SOURCE_DIR}/inc;LD_LIBRARY_PATH=${CMAKE_BINARY_DIR}"
+)
+
+add_executable(RamviewCountsTest RamviewCountsTest.cxx)
+target_link_libraries(RamviewCountsTest PRIVATE
+    ramtools_tree
+    ramtools_ntuple
+    gtest
+    gtest_main
+)


Can we move this into a cmake function? You can take a look at how it's done in clad...

.clang-format

Signed-off-by: AdityaPandeyCN <[email protected]>

vgvassilev · 2025-07-30T14:01:10Z

CMakeLists.txt

+set(CMAKE_INSTALL_LIBDIR lib)
+set(CMAKE_INSTALL_INCLUDEDIR include)
+
+set_property(DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR} PROPERTY INCLUDE_DIRECTORIES ${CMAKE_CURRENT_SOURCE_DIR}/inc)


That's the same as include_directories. Why do we need this to be set globally?

I am still confused about this, not able to implement removing include directories.

I will need more information about that. Compiling which library fails if you remove that line?

Made some changes, what do you think about it now?

vgvassilev · 2025-07-30T14:02:58Z

test/CountEntriesTest.cxx

+#include <gtest/gtest.h>
+#include <TSystem.h>
+#include <TFile.h>
+#include <TTree.h>
+#include <ROOT/RNTupleReader.hxx>
+#include <iostream>
+#include <string>


The includes are not ordered by component and then alphabetically.

vgvassilev · 2025-07-30T14:04:53Z

test/CountEntriesTest.cxx

+    }
+};
+
+TEST_F(CountEntriesTest, CompareEntryCountsFromSamFile) {


I guess I do not understand what is this intended to test. Nominally the content of samtoram.cxx should be turned into a test (or some library api that we call from within the test). Then in the benchmark folder we can do all sorts of benchmarks and comparisons...

I intented to test count of reads for a genomic region from both the implementation rntuple one and the ttree one.

Some part of the test log to give you an overview
RAMRefs vector: 1: 0: chr1 1: RAMRefs vector: 1: 1: Processed 24 SAM headers 1: Processed 33711 SAM records 1: 1: Real time 0:00:01, CP time 1.800 1: 1: Processing samtoramntuple.cxx+("samexample.sam", "rntuple_test.root")... 1: 1: RAM file created: rntuple_test.root 1: Number of entries: 33711 1: RAMNTupleRefs vector: 1: 0: chr1 1: RAMNTupleRefs vector: 1: 1: Index entries: 34 1: 1: Processed 24 SAM headers 1: Processed 33711 SAM records

Alright I understood what you mean to say, I will implement that. Thankyou for this

Should I go on and create a library folder with the content of samtoram.cxx and samtoramntuple.cxx and use them to perform unit tests and move these two in the benchmark folder(with slight modifications)?

Probably a good idea. We will need to meditate on a name of that library though :)

vgvassilev · 2025-07-30T14:05:16Z

test/RamviewCountsTest.cxx

+    }
+};
+
+TEST_F(RamviewCountsTest, CompareViewCountsForRegion) {


Signed-off-by: AdityaPandeyCN <[email protected]>

AdityaPandeyCN · 2025-08-12T11:04:35Z

Hello @vgvassilev Do you want me to address anything more for this PR?

Signed-off-by: AdityaPandeyCN <[email protected]>

CMakeLists.txt

vgvassilev · 2025-08-13T06:56:32Z

CMakeLists.txt

 )

-ROOT_EXECUTABLE(samtoramntuple


Please use the ROOT_EXECUTABLE where possible.

tools/samtoram.cxx

Signed-off-by: AdityaPandeyCN <[email protected]>

Co-authored-by: Vassil Vassilev <[email protected]>

inc/ramcore/SamParser.h

vgvassilev · 2025-08-13T12:03:45Z

inc/ramcore/SamToNTuple.h

@@ -1,5 +1,5 @@
-#ifndef RAMCORE_SAMTONTUPLE_H
-#define RAMCORE_SAMTONTUPLE_H
+#ifndef ramcore_SAMTONTUPLE_H


Header protectors should be all caps.

Signed-off-by: AdityaPandeyCN <[email protected]>

AdityaPandeyCN marked this pull request as draft July 23, 2025 15:12

resolve merge conflicts

2ff8918

Signed-off-by: AdityaPandeyCN <[email protected]>

AdityaPandeyCN marked this pull request as ready for review July 23, 2025 15:41

AdityaPandeyCN added 2 commits July 23, 2025 21:14

removed redundant files

f0e0ef4

Signed-off-by: AdityaPandeyCN <[email protected]>

removed redundant files

e172568

Signed-off-by: AdityaPandeyCN <[email protected]>

AdityaPandeyCN marked this pull request as draft July 23, 2025 16:16

conflict changes

650e549

Signed-off-by: AdityaPandeyCN <[email protected]>

AdityaPandeyCN marked this pull request as ready for review July 24, 2025 03:56

vgvassilev reviewed Jul 28, 2025

View reviewed changes

addressed wrong imports

cbad6b8

Signed-off-by: AdityaPandeyCN <[email protected]>

vgvassilev reviewed Jul 28, 2025

View reviewed changes

AdityaPandeyCN added 3 commits July 29, 2025 11:48

making imports more clear and updating CMakeLists

2813225

Signed-off-by: AdityaPandeyCN <[email protected]>

making imports better

34e54c8

Signed-off-by: AdityaPandeyCN <[email protected]>

fixing imports

dc6da60

Signed-off-by: AdityaPandeyCN <[email protected]>

vgvassilev reviewed Jul 29, 2025

View reviewed changes

Fixed imports and addressed CMakeLists suggestions

4755ba1

Signed-off-by: AdityaPandeyCN <[email protected]>

vgvassilev reviewed Jul 30, 2025

View reviewed changes

merged ttree and rntuple libraries

5c35c39

Signed-off-by: AdityaPandeyCN <[email protected]>

vgvassilev reviewed Jul 30, 2025

View reviewed changes

Extract tool logic into a core library with tests

f455f2b

Signed-off-by: AdityaPandeyCN <[email protected]>

changes to cmake

b1c3f48

Signed-off-by: AdityaPandeyCN <[email protected]>

vgvassilev reviewed Aug 13, 2025

View reviewed changes

renamed ramcore library

baf0205

Signed-off-by: AdityaPandeyCN <[email protected]>

Update ROOT version in CMakeLists.txt

9b281a0

Co-authored-by: Vassil Vassilev <[email protected]>

vgvassilev reviewed Aug 13, 2025

View reviewed changes

inc/ramcore/SamParser.h Show resolved Hide resolved

vgvassilev reviewed Aug 13, 2025

View reviewed changes

added docs for main classes

60fdd9e

Signed-off-by: AdityaPandeyCN <[email protected]>


		add_library(ramtools_tree STATIC
		include_directories(${CMAKE_CURRENT_SOURCE_DIR}/inc/ttree)

		@@ -1,44 +1,75 @@
		cmake_minimum_required(VERSION 3.16)
		project(ramtools VERSION 1.0.0 LANGUAGES C CXX)

		@PACKAGE_INIT@

		include("${CMAKE_CURRENT_LIST_DIR}/ramtoolsTargets.cmake")

		)

		ROOT_EXECUTABLE(samtoramntuple

		)

		ROOT_EXECUTABLE(samtoramntuple

Restructuring #2

Are you sure you want to change the base?

Restructuring #2

Uh oh!

Conversation

AdityaPandeyCN commented Jul 23, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AdityaPandeyCN commented Jul 30, 2025

Uh oh!

vgvassilev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!