[tmva][sofie] Add TopK operator #15886

lmoneta · 2024-06-19T16:36:08Z

This Pull request adds the support for TopK operator in SOFIE

Implementation provided by GSOC student from Vedant Mehra.

This PR is based on #15837 and should be merged after that one and eventually rebased

sanjibansg · 2024-06-19T17:53:28Z

tmva/sofie/inc/TMVA/ROperator_Reduce.hxx

+      //std::cout << "Reduce operator - axis = " << fAttrAxes[0] << " shape x " << ConvertShapeToString(fShapeX)
+      //          << " output shape " << ConvertShapeToString(fShapeY) << std::endl;


maybe we can remove this commented code?

sanjibansg · 2024-06-19T17:55:21Z

tmva/sofie/inc/TMVA/ROperator_TopK.hxx

+      // if(fK>fShapeX[fAttrAxis]){
+      //    throw
+      //       std::runtime_error("TMVA::SOFIE ONNX TopK op k = "+ std::to_string(fK) +" value exeeds value of tensor " +fNX+" of size "+fShapeX.size()+" at axis= "+std::to_string(fAttrAxis)+".");
+      // }
+      // fShapeX = model.GetTensorShape(fNX); //  [ m x n x o x p ... ]
+      // if(k[0]>=fShapeX.size()){
+      //    throw
+      //       std::runtime_error("TMVA::SOFIE ONNX TopK op k = "+ std::to_string(k[0]) +"value exeeds size of tensor " +fNX+" of size "+fShapeX.size()+" .");
+      // }
+      // fShapeY.push_back(2);
+      // for (auto i : fShapeX)
+      //    fShapeY.push_back(i); //  [ 2 x m x n x o x p ... ]
+      // size_t axis = fAttrAxis < 0 ? fShapeX.size() + fAttrAxis : fAttrAxis;
+      // fShapeY[axis] = k[0]; //  [ 2 x m x n x K x p ... ]


same with the commented code here

sanjibansg · 2024-06-19T17:56:16Z

tmva/sofie/src/RModel.cxx

@@ -797,7 +833,7 @@ long RModel::WriteInitializedTensorsToFile(std::string filename) {
 void RModel::PrintRequiredInputTensors() {
    std::cout << "Model requires following inputs:\n";
    for (auto& inputInfo: fInputTensorInfos) {
-        std::cout << "Parameterised Tensor name: " << inputInfo.first << "\t";
+        std::cout << "Parametraised Tensor name: " << inputInfo.first << "\t";


Suggested change

std::cout << "Parametraised Tensor name: " << inputInfo.first << "\t";

std::cout << "Parameterised Tensor name: " << inputInfo.first << "\t";

github-actions · 2024-06-19T18:38:33Z

Test Results

13 files 13 suites 2d 13h 35m 1s ⏱️
2 650 tests 2 649 ✅ 0 💤 1 ❌
32 634 runs 32 633 ✅ 0 💤 1 ❌

For more details on these failures, see this check.

Results for commit 449baf1.

♻️ This comment has been updated with latest results.

dpiparo

Thanks for this PR. The code seems to do what it's supposed to, i.e. add an operator. However the code seems to need some refactoring, for example avoiding to pass by value potentially large objects, by not having commented code, unclear functions.

dpiparo · 2024-06-20T13:01:20Z

tmva/sofie/inc/TMVA/RModel.hxx

@@ -60,6 +60,9 @@ public:
   }
   void AddInitializedTensor(std::string tensor_name, ETensorType type, std::vector<std::size_t> shape,
                             std::shared_ptr<void> data);
+   void AddConstantTensor(std::string tensor_name, ETensorType type, std::vector<std::size_t> shape,


perhaps strings have to be passed by const ref?

This is a comment valid for all signatures featuring string.

dpiparo · 2024-06-20T13:02:24Z

tmva/sofie/inc/TMVA/ROperator_Constant.hxx

+public:
+   ROperator_Constant(){}
+
+   ROperator_Constant(const std::string & type, const std::vector<T> & values, const std::vector<size_t> & shape, std::string nameX, std::string nameY):


here vectors are passed properly, but not strings. see above.

dpiparo · 2024-06-20T13:02:36Z

tmva/sofie/inc/TMVA/ROperator_Constant.hxx

+      fAttrType(type)
+      { }
+
+   std::vector<ETensorType> TypeInference(std::vector<ETensorType> input){


dpiparo · 2024-06-20T13:05:54Z

tmva/sofie/inc/TMVA/ROperator_Constant.hxx

+      return input;
+   }
+
+   std::vector<std::vector<size_t>> ShapeInference(std::vector<std::vector<size_t>> input){


Is the goal of this function to make a copy of the input? The implementation also is strange, right? Why not just making a copy instead of calling it?

dpiparo · 2024-06-20T13:06:59Z

tmva/sofie/inc/TMVA/ROperator_Constant.hxx

+}//TMVA
+
+
+#endif //TMVA_SOFIE_ROPERATOR_Constant


missing trailing carriage return.

dpiparo · 2024-06-20T13:08:33Z

tmva/sofie/inc/TMVA/ROperator_Reduce.hxx

@@ -56,102 +58,120 @@ public:
   // shape of output tensors given input tensors
   std::vector<std::vector<size_t>> ShapeInference(std::vector<std::vector<size_t>> input){


again, this syntax is misleading. Why not passing a const ref?

dpiparo · 2024-06-20T13:09:44Z

tmva/sofie/inc/TMVA/ROperator_Reduce.hxx


-    std::string Generate(std::string OpName){
+   std::string Generate(std::string OpName){


dpiparo · 2024-06-20T13:10:12Z

tmva/sofie/inc/TMVA/ROperator_TopK.hxx

+        fNVal(UTILITY::Clean_name(nameVal)),
+        fNInd(UTILITY::Clean_name(nameInd)){}
+
+   std::vector<ETensorType> TypeInference(std::vector<ETensorType> input) {


again const ref.

dpiparo · 2024-06-20T13:11:25Z

tmva/sofie/src/RModel.cxx

+         size_t length = ConvertShapeToLength(i.second.shape());
+            // in case we are not using weight files or for tensor created from Constant operator
+         if (!fUseWeightFile || i.second.IsConstantTensor() ) {
+            //std::cout << "write tensor " << i.first << std::endl;


These changes contain commented code, should it be removed?

dpiparo · 2024-06-20T13:14:08Z

tmva/sofie/src/RModel.cxx

+               strs << "float tensor_" << i.first << "[" << length << "] = {";
+               float const *data = i.second.data<float>();
+               for (size_t idx = 0; idx < length; idx++) {
+                  strs << std::setprecision(std::numeric_limits<float>::max_digits10) << data[idx];


a general comment: should the fp numbers be written in hex format not to loose any precision?

Implement TopK operator (work from GSOC student Vedant Mehra)

Fix the output type when parsing TopK Clean up the code in TopK impelmentation and in the generated code. Fix also the compilation warnings

lmoneta requested a review from bellenot as a code owner June 19, 2024 16:36

lmoneta requested a review from sanjibansg June 19, 2024 16:36

sanjibansg reviewed Jun 19, 2024

View reviewed changes

dpiparo self-requested a review June 20, 2024 13:00

dpiparo requested changes Jun 20, 2024

View reviewed changes

dpiparo assigned lmoneta Jun 20, 2024

lmoneta added 2 commits June 26, 2024 14:16

[tmva][sofie] Add TopK operator (from Vedant Mehra)

77c0ff2

Implement TopK operator (work from GSOC student Vedant Mehra)

[tmva][sofie] Fix output type of TopK and clean up code

449baf1

Fix the output type when parsing TopK Clean up the code in TopK impelmentation and in the generated code. Fix also the compilation warnings

lmoneta force-pushed the tmva_sofie_add_topk_operator branch from 3d54e72 to 449baf1 Compare June 26, 2024 12:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[tmva][sofie] Add TopK operator #15886

[tmva][sofie] Add TopK operator #15886

lmoneta commented Jun 19, 2024

sanjibansg Jun 19, 2024

sanjibansg Jun 19, 2024

sanjibansg Jun 19, 2024

github-actions bot commented Jun 19, 2024 •

edited

Loading

dpiparo left a comment

dpiparo Jun 20, 2024

dpiparo Jun 20, 2024

dpiparo Jun 20, 2024

dpiparo Jun 20, 2024

dpiparo Jun 20, 2024

dpiparo Jun 20, 2024

dpiparo Jun 20, 2024

dpiparo Jun 20, 2024

dpiparo Jun 20, 2024

dpiparo Jun 20, 2024

dpiparo Jun 20, 2024

		//std::cout << "Reduce operator - axis = " << fAttrAxes[0] << " shape x " << ConvertShapeToString(fShapeX)
		// << " output shape " << ConvertShapeToString(fShapeY) << std::endl;

	std::cout << "Parametraised Tensor name: " << inputInfo.first << "\t";
	std::cout << "Parameterised Tensor name: " << inputInfo.first << "\t";

		@@ -56,102 +58,120 @@ public:
		// shape of output tensors given input tensors
		std::vector<std::vector<size_t>> ShapeInference(std::vector<std::vector<size_t>> input){


		std::string Generate(std::string OpName){
		std::string Generate(std::string OpName){

		}//TMVA


		#endif //TMVA_SOFIE_ROPERATOR_Constant

[tmva][sofie] Add TopK operator #15886

Are you sure you want to change the base?

[tmva][sofie] Add TopK operator #15886

Conversation

lmoneta commented Jun 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jun 19, 2024 • edited Loading

Test Results

dpiparo 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

github-actions bot commented Jun 19, 2024 •

edited

Loading