[tmva][sofie] ROperator_Comparision on-the-fly broadcasting #19739
Conversation
| @@ -77,7 +77,7 @@ public: | |||
| ROperator_Comparision(const std::string & nameX1, const std::string & nameX2, const std::string & nameY): | |||
There was a problem hiding this comment.
@lmoneta do we need the std::vector<Dim> fDimShapeX1; std::vector<Dim> fDimShapeX2; here? They seem to be only used in two if cases
Test Results 19 files 19 suites 3d 4h 7m 53s ⏱️ For more details on these failures, see this check. Results for commit b6041d8. |
There was a problem hiding this comment.
Thank you for fixing also this operator to avoid broadcasting and for adding the relevant test.
Apart for some issue of some deleted code which should be still valid, the other changes are for following and being consistent what has been done in the BinaryOperator:
https://github.com/root-project/root/pull/19742/files
| @@ -113,6 +113,8 @@ public: | |||
| fShapeX2 = model.GetTensorShape(fNX2); | |||
| fDimShapeX2 = ConvertShapeToDim(fShapeX2); | |||
| } | |||
| fShapeX1 = model.GetTensorShape(fNX1); | |||
| fShapeX2 = model.GetTensorShape(fNX2); | |||
There was a problem hiding this comment.
This is a duplication of the 107 and 113 lines above
| } else if (model.IsShapeTensor(fNX2)) { | ||
| shapeData2 = model.GetShapeTensorValues(fNX2); | ||
| } | ||
| if (data1 && data2) { |
| } | ||
| } | ||
| delete [] outData; | ||
| if (!fIsOutputConstant) { |
There was a problem hiding this comment.
Also this code should be kept and not deleted!
| } | ||
|
|
| out << SP << SP << "delete[] data;\n"; | ||
| out << SP << "}\n"; | ||
|
|
||
| auto stridesX1 = UTILITY::ComputeStrideFromShape(fShapeX1); |
There was a problem hiding this comment.
Here we should use fDimShapeX1 and fDimShapeX2 instead of fShapeX1 and fShapeX2
| } else { | ||
| for(size_t i = 0; i<fShapeX1.size(); ++i){ | ||
| if(fShapeX1[i]==1) continue; | ||
| compute_idx_X1 += " idx_"+fNY+std::to_string(i+(fShapeY.size()-fShapeX1.size()))+" * "+std::to_string(stridesX1[i])+" +"; |
There was a problem hiding this comment.
No need to add "fNY" to the definition of the index variables. It will pollute the name, better to keep it simple
There was a problem hiding this comment.
Also it would be nice to distinguish the case when the strides are equal to 1. In this case it is not needed to add a multiplication with 1 : " * 1". See examples at line 332 in https://github.com/root-project/root/pull/19742/files
| auto stridesY = UTILITY::ComputeStrideFromShape(fShapeY); | ||
|
|
||
| std::string compute_idx_X1, compute_idx_X2, compute_idx_Y; | ||
| if (std::all_of(fShapeX1.begin(), fShapeX1.end(), [](size_t x) { return x == 1; })){ |
There was a problem hiding this comment.
This needs to be changed for the case of fDimShapeX1, see
https://github.com/root-project/root/pull/19742/files
|
|
||
| for(size_t i=0; i<fShapeY.size(); ++i){ | ||
| if(fShapeY[i]!=1){ | ||
| out<<std::string(i + 1, ' ')<<"for(size_t idx_"<<fNY<<i<<"=0; idx_"<<fNY<<i<<"<"<<fShapeY[i]<<"; ++idx_"<<fNY<<i<<"){\n"; |
There was a problem hiding this comment.
For fixing the indentention space of the generated code see again:
https://github.com/root-project/root/pull/19742/files
This Pull request:
This pull request refactors the ROperator_Comparision to handle broadcasting more efficiently.
Added new tests to verify the correctness of the ROperator_Comparision.