Binary op (for testing) #19

lmoneta · 2022-06-23T14:28:28Z

This Pull request:

Changes or fixes:

Checklist:

tested changes locally
updated the docs (if necessary)

This PR fixes #

lmoneta

Thank you for the code.
It works after following the comments.
I will send you also the fixed code on Mattermost.

Lorenzo

lmoneta · 2022-06-23T14:37:52Z

tmva/sofie/inc/TMVA/RModel_BasicBinaryOp.hxx

 private:

   std::string fNX1;
   std::string fNX2;
   std::string fNY;
   std::vector<size_t> fShape;
+   template <typename T, EBasicBinaryOperator Op1>


I would define these template struct outside the class definition, before line 16, because there could be some issues with partial template specialisations for internal classes

lmoneta · 2022-06-23T14:40:02Z

tmva/sofie/inc/TMVA/RModel_BasicBinaryOp.hxx

 private:

   std::string fNX1;
   std::string fNX2;
   std::string fNY;
   std::vector<size_t> fShape;
+   template <typename T, EBasicBinaryOperator Op1>
+   struct BinaryOperatorTrait {
+      const char * Name() { return "";}


All these should be static functions. DO:

static const char * Name() {....} static const char * Op() {....}

lmoneta · 2022-06-23T14:46:08Z

tmva/sofie/inc/TMVA/RModel_BasicBinaryOp.hxx

      }
      std::stringstream out;
      // int length = 1;
      // for(auto& i: fShape){
      //    length *= i;
      // }
      size_t length = ConvertShapeToLength(fShape);
-      out << "\n//------ Add\n";
+      out << "\n//------ " + s->Name()+"\n";


use here:

BinaryOperatorTrait<T,Op>::Name()

and 2 lines below

BinaryOperatorTrait<T,Op>::Op()

Also here we should use std::string. Maybe we define the functions to return a std::string instead of doing
std::string(BinaryOperatorTrait<T,Op>::Op())

lmoneta · 2022-06-23T14:46:48Z

tmva/sofie/inc/TMVA/RModel_BasicBinaryOp.hxx

-template <typename T>
-class ROperator_Add final : public ROperator
-{
+enum EBasicBinaryOperator { Add, Sub, Mul, Div };



You need to make it a template class, add this line:

template <typename T, EBasicBinaryOperator Op>

And the class should not be called RModel_BasicBinaryOp, but to be consistent:
ROperator_BasicBinary

lmoneta · 2022-06-23T14:56:24Z

tmva/sofie_parsers/inc/TMVA/RModelParser_ONNX.hxx

@@ -35,7 +35,7 @@ std::unique_ptr<ROperator> make_ROperator_RNN(const onnx::NodeProto& nodeproto,
 std::unique_ptr<ROperator> make_ROperator_LSTM(const onnx::NodeProto& nodeproto, const onnx::GraphProto& graphproto, std::unordered_map<std::string, ETensorType>& tensor_type);
 std::unique_ptr<ROperator> make_ROperator_BatchNormalization(const onnx::NodeProto& nodeproto, const onnx::GraphProto& graphproto, std::unordered_map<std::string, ETensorType>& tensor_type);
 std::unique_ptr<ROperator> make_ROperator_Pool(const onnx::NodeProto& nodeproto, const onnx::GraphProto& graphproto, std::unordered_map<std::string, ETensorType>& tensor_type);
-std::unique_ptr<ROperator> make_ROperator_Add(const onnx::NodeProto &nodeproto, const onnx::GraphProto &graphproto, std::unordered_map<std::string, ETensorType> &tensor_type);
+std::unique_ptr<ROperator> make_RModel_BasicBinaryOp(const onnx::NodeProto &nodeproto, const onnx::GraphProto &graphproto, std::unordered_map<std::string, ETensorType> &tensor_type);


see comment on different name for class
(use ROperator_BasicBinary}

lmoneta · 2022-06-23T14:57:16Z

tmva/sofie_parsers/src/RModelParser_ONNX.cxx

@@ -48,7 +48,7 @@ std::unique_ptr<ROperator> make_ROperator_Add(const onnx::NodeProto& nodeproto,
      op.reset(new ROperator_Add<float>(nodeproto.input(0), nodeproto.input(1), nodeproto.output(0)));


do:

op.reset(new ROperator_BasicBinary<float,Add>

Such pointer will be automatically released when unloading library or program is stopped

In commit 196ef3b, `operator=` for the RooPolyFunc was implemented, but it was implemented wrognly: it used the assignment operator the the proxy collections, which does value syncronization and not copy assignment! It's better to delete the copy assignment for RooPolyFunc, because copy/move assignment of RooFit objects is very badly supported.

There was a bug in a schema evolution rule for a very old RooProduct version, where a `RooSetProxy` member was replaced by a `RooListProxy`. The problem was that only the content was copied to the new proxy list, not the other members. This lead to an invalid state for the RooSetProxy, because it had no registered owner. Commit aa55e5c attempted to fix the problem, but the fix used the `RooArgList::operator=`, which doesn't implement copy assignment, but value synchronization! So the target proxy list was empty after calling it. Therefore, one the RooCollectionProxy needs to be extended with something that does exactly what the schema evolution requires: setting the owner of the proxy and adding all elements without registering the elements as servers (the server lists are copied separately by IO). A new member function initializeAfterIOConstructor` was added to the RooCollectionProxy for this. In the initial attempt to the fix, it was thought the original problem was only that the proxy owner was not set in the proxy, but actually the _proxyList only contained `nullptr` after reading and old RooProduct! The reason for that is the reason for which `RooAbsArg::ioStreamerPass2()` exists (quoting from its documentation): > second pass is typically needed when evolving data member of > RooAbsArg-derived classes that are container classes with references > to other members, which may not yet be 'live' in the first > ioStreamer() evolution pass. Hence, `RooProduct` needs to override `ioStreamerPass2()` and set the proxy list correctly there, which is also done in this commit.

The documentation of `RooAbsArg::ioStreamerPass2()` says: > Classes may overload this function, but must call the base method in > the overloaded call to ensure base evolution is handled properly This was not done in the overload in RooHistFunc, which is a mistake that probably leads to backwards compatibility errors that are undetected to far.

Two of the previous commits have shown that copy assignment for RooCollectionProxy is dangerous because it behaves unexpectedly. However, it is not clear how it *should* behave. Should default assignment be used? But then, it will use the assignment operators of the RooFit collections, which actually don't do assignment, but value syncronization! Should it be re-implemented to be actual assignment? That would be inconsistent with the base class! So it's better to not support it at all.

This commit renames and refactors `RDaosEventQueue` to include: - Parent events that are responsible for a batch of asynchronous events originating from a single RW call. This simplifies polling, which is a waiting call on the batch of requests sent under that parent that are inflight (`WaitOnParentBarrier`, which sets a parent event barrier and waits for all children launched before it to complete). - Poolwide event queues that are shared among its underlying containers and persist between RW calls. This eliminates the significant overhead behind instantiating new queues for each call to `VectorReadWrite`. - Queue management obeying RAII principle. - Better handling of synchronous events in `FetchUpdateArgs` (the default behavior). The event is an optional attribute which yields a nullptr to DAOS fetch/update calls in such cases, indicating the request is blocking. - The functions `daos_event_test` and `daos_event_parent_barrier`, used for polling and sending parent events respectively, to `libdaos_mock`. Co-authored-by: Giovanna Lazzari Miotto <[email protected]> Co-authored-by: Javier Lopez-Gomez <[email protected]>

Ensure that buffer will be deleted when painter is deleted

All contour plots was not releasing that painter at the end

Ensure that allocated memory always released

Special "contours" object which is created in with "list" draw option, was not fully deleted when option used again

Ensure that allocated memory released at the end in any circumstances

Also avoid comparation obj->TestBit(bit) == 0 or == 1, just use in logical operators

Inline C++ needs double quotes, Python is ok with single quotes.

We now register all RDefine nodes, incuding varied defines, with RLoopManager, which is in charge of calling InitSlot on all registered nodes, so we do not need to propagate the InitSlot call from RVariation or RDefine to other RDefines.

Temporary TBox object was not deleted, now just in stack

Temporary TText object was not deleted, now used in stack

It is good practice to cleanup memory at macro exit

Original object was not deleted properly

This is to get rid of deprecation warninigs when building on Ubuntu 22.04. Should cause no backwards compatibility poblems, as the functions that are used now are around at least since OpenSSL 1.0.2: https://www.openssl.org/docs/man1.0.2/man3/EVP_DigestInit_ex.html This patch was already applied to upstream civet: civetweb/civetweb#1072

Keep old code for Windows and older SSL versions

…d and 3d case (root-project#10768) * Fix the issue related to 1d and 3d MaxPool operators * Update ROperator_Pool.hxx * The issue related to Maxpool for 1d and 3d fixed * Test related to MaxPool 1d added * MaxPool Operator fixed for 1d and 3d cases and tests added * Max Pool operator errors related to 1d and 3d case resolved * Max Pool operator errors related to 1d and 3d case resolved * Updated the Pool Operator * Fix warning in new implementation of Pool operator * Added the tests for MaxPool 2d and 3d Operators * Fix 2d and 3d MaxPool operators Use the correct stride for the height when loopoing at the tensor element. The height stride for 2d is equal to wsize, and in 3d is equal to wsize*dsize. Precompute the hstride in 3d to avoid a multiplication when looping through the tensor elements. * Tests added for AvgPool * Corrected the spelling errors Co-authored-by: moneta <[email protected]>

If the proxy normSet changed, we also have to set the value dirty flag of the proxy owner. Otherwise, value for the new normalization set might not get recomputed, which causes bugs! The issue can be reproduced with this small code snippet: ```C++ using namespace RooFit; RooRealVar x("x", "x", 0, -10, 10); RooGaussian gauss("gauss", "gauss", x, RooConst(0), RooConst(2)); RooAddition add("add", "add", {gauss}); std::cout << add.getVal() << std::endl; std::cout << add.getVal(x) << std::endl; ``` Without this commit, the value will be the same with and without normalization set, because changing only the normalization set didn't trigger a recomputation. This code snippet has also been implemented as a unit test now, where it is tested that the values are different.

Much of the functionality of RooAddition is implemented in exactly the same way as in RooRealSumPdf. Hence, to avoid code duplication, we can reuse the static functions in RooRealSumPdf that provide this implementation.

Much of the functionality of RooAddPdf is implemented in exactly the same way as in RooRealSumPdf. Hence, to avoid code duplication, we can reuse the static functions in RooRealSumPdf that provide this implementation.

Use more clear std::make_unique calls

Created list of functions and list of parameters names was not deleted properly.

…oot-project#10435) * Some error messages and spelling mistakes fixed related to operaters * Update the indentation of BatchNormalization.hxx Co-authored-by: Neel Shah <neelshah29042002.com>

…ponding unit tests and Multi-directional broadcasting functionality is added for SOFIE

lmoneta commented Jun 23, 2022

View reviewed changes

Neel-Shah-29 force-pushed the BinaryOp branch from cf7ba5d to 9c9dd83 Compare June 24, 2022 06:14

alja and others added 28 commits June 24, 2022 08:15

Handle selection for filtered digits. Enable custom tooltip

9a613a2

Add check if digit is visible in FillExtraSelectionData

7b524be

Add example for RecHit proxy buiders

9ccf060

Remove debug prints

c8e617e

Add check for empty boxset in makeBoxSet()

5547dec

Changes comments

11c49f2

[asimage] use unique_ptr for TBox created in ProcessEvent

b386d38

Such pointer will be automatically released when unloading library or program is stopped

use std::vector to keep temporary array in TPolyLine::ExecuteEvent

0250f55

Use std::unique_ptr for temporary TBox in TPad::ExecuteEventAxis

7488ebd

Use std::vector<Int_t> for temp array in TGraph::ExecuteEvent

0c34796

Use unique_ptr<TBox> for temp object in TH1::ExecuteEvent

045129c

Use std::vector<Int_t> for static list of colors in TImagePalette

3168b31

Use std::unique_ptr<TBox> for static objects in THistPainter

47593d6

[skip-ci] put CONT5 option in the right place (root-project#10819)

98d05db

[histpainter] use std::vector<Double_t> for interim buffer

468d20a

Ensure that buffer will be deleted when painter is deleted

Fix memory leak with TGraph2DPainter allocated in hist painter

8befe27

All contour plots was not releasing that painter at the end

Use std::vectors in histpainter contour plot

7420244

Ensure that allocated memory always released

Fix memory leak with "cont list" draw option

28bba1e

Special "contours" object which is created in with "list" draw option, was not fully deleted when option used again

Use std::vector in THistPainter methods instead of plain arrays

e1ddc19

Ensure that allocated memory released at the end in any circumstances

[histpainter] use nullptr, avoid Form(), simplify TIter loops

de4a1b3

Also avoid comparation obj->TestBit(bit) == 0 or == 1, just use in logical operators

[NFC][DF] Fix typo in Python example

011a98d

Inline C++ needs double quotes, Python is ok with single quotes.

[DF] Remove redundant calls to RDefine::InitSlot

45d1530

We now register all RDefine nodes, incuding varied defines, with RLoopManager, which is in charge of calling InitSlot on all registered nodes, so we do not need to propagate the InitSlot call from RVariation or RDefine to other RDefines.

[NFC][DF] Consistently use the imperative mood in docs

9c440e5

linev and others added 26 commits June 29, 2022 16:37

Use nullptr in TPad.cxx

c6fe4f5

Fix memory leak in TPad::DrawCollideGrid and TPad::DrawColorTable

5fca747

Temporary TBox object was not deleted, now just in stack

Fix memory leaks in tmvagui

9bce4e9

Temporary TText object was not deleted, now used in stack

Fix temporary TText leaking in tutorials

b306a89

Fix TLatex memory leak in stressGraphics

792d9b5

Fix TLatex memory leak in tutorials and demos

4b38e64

Fix TLine::DrawLine memory leaks in tutorials and demos

a8567a3

Fix TArrow::DrawArrow memory leak in tutorials

702cf95

Fix TMarker::DrawMarker memory leaks in spectrum tutorials

afa57c5

Release memory and dynamic objects in TSpectrum2 tutorials

b911b0b

It is good practice to cleanup memory at macro exit

Fix TArc::DrawArc related leaks

5c58340

Fix leak TPaveLabel::DrawPaveLabel usage

4a4b27b

Original object was not deleted properly

[tree] Add interface to add a friend in RFriendInfo

d18d368

[tree][NFC] Clarify comment

3febf4a

[tree][NFC] make formatter happy

ed6f3bf

[civetweb] use EVP_Digest only for OPENSSL 3.0

08c41c6

Keep old code for Windows and older SSL versions

[RF] Avoid duplication of RooRealSumPdf code in RooAddition

1f0d5ad

Much of the functionality of RooAddition is implemented in exactly the same way as in RooRealSumPdf. Hence, to avoid code duplication, we can reuse the static functions in RooRealSumPdf that provide this implementation.

[RF] Avoid duplication of RooRealSumPdf code in RooAddPdf

6fa5c44

Much of the functionality of RooAddPdf is implemented in exactly the same way as in RooRealSumPdf. Hence, to avoid code duplication, we can reuse the static functions in RooRealSumPdf that provide this implementation.

Protect TF1 constructor against nullptr as formula parameter

61a102d

Use more clear std::make_unique calls

Fix memory leak in TF1 constructor NSUM(...) parsing

1d20621

Created list of functions and list of parameters names was not deleted properly.

Protect hist classes against calling strlen with nullptr argument

8baea4c

Some error messages and spelling mistakes fixed related to operators (r…

4e0ef33

…oot-project#10435) * Some error messages and spelling mistakes fixed related to operaters * Update the indentation of BatchNormalization.hxx Co-authored-by: Neel Shah <neelshah29042002.com>

All the four Binary Operators:- Add,Sub,Mul,Div added with the corres…

f799ed3

…ponding unit tests and Multi-directional broadcasting functionality is added for SOFIE

Neel-Shah-29 force-pushed the BinaryOp branch from 5c5c03a to f799ed3 Compare June 30, 2022 11:03

Neel-Shah-29 added 2 commits June 30, 2022 21:33

Required changes related to comments are done.

9489b33

Required changes made for the comments

6df2e5e

Neel-Shah-29 deleted the BinaryOp branch July 1, 2022 11:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Binary op (for testing) #19

Binary op (for testing) #19

lmoneta commented Jun 23, 2022

lmoneta left a comment

lmoneta Jun 23, 2022

lmoneta Jun 23, 2022

lmoneta Jun 23, 2022

lmoneta Jun 23, 2022

lmoneta Jun 23, 2022

lmoneta Jun 23, 2022

lmoneta Jun 23, 2022

lmoneta Jun 23, 2022

		@@ -48,7 +48,7 @@ std::unique_ptr<ROperator> make_ROperator_Add(const onnx::NodeProto& nodeproto,
		op.reset(new ROperator_Add<float>(nodeproto.input(0), nodeproto.input(1), nodeproto.output(0)));

Binary op (for testing) #19

Are you sure you want to change the base?

Binary op (for testing) #19

Conversation

lmoneta commented Jun 23, 2022

This Pull request:

Changes or fixes:

Checklist:

lmoneta 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