Develop #25

JuanBSLeite · 2018-03-09T17:02:01Z

No description provided.

AAAlvesJr · 2018-03-10T09:55:48Z

Olá Juan, thanks for this PR.

I see a lot of files in your request. Could you reconfigure the PR and push only your contribution?
The file implementing the Flatté. Ideally, please, implement the line-shape in a header file ".h" (as you already did) and add some documentation citing also a reference from where you got the formulae. Then provide one or two examples placed in the folder "examples/phys" and add the target(s) to "examples/phys/CMakeList.txt". Creating the examples/NIPS_HYDRA is not necessary. There are some .xml files, and a .idea subdirectory, I suppose from your IDE (CLION ?). Please exclude then. Change fDaughter3Mass and the corresponding getter/setter members to fBachelorMass. I discovered recently that it keep as fDaughter3Mass can be painfully misleading...

Cheers
A.A.
.

JuanBSLeite · 2018-03-10T14:19:27Z

Hi Augusto, OK, I'll do the modifications and do the PR again. Att, Juan Leite

…

________________________________ From: Antonio Augusto Alves Junior <notifications@github.com> Sent: Saturday, March 10, 2018 9:55 AM To: MultithreadCorner/Hydra Cc: JuanBSLeite; Author Subject: Re: [MultithreadCorner/Hydra] Develop (#25) Olá Juan, thanks for this PR. I see a lot of files in your request. Could you reconfigure the PR and push only your contribution? The file implementing the Flatté. Ideally, please, implement the line-shape in a header file ".h" (as you already did) and add some documentation citing also a reference from where you got the formulae. Then provide one or two examples placed in the folder "examples/phys" and add the target(s) to "examples/phys/CMakeList.txt". Creating the examples/NIPS_HYDRA is not necessary. There are some .xml files, and a .idea subdirectory, I suppose from your IDE (CLION ?). Please exclude then. Change fDaughter3Mass and the corresponding getter/setter members to fBachelorMass. I discovered recently that it keep as fDaughter3Mass can be painfully misleading... Cheers A.A. . — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub<https://eur02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FMultithreadCorner%2FHydra%2Fpull%2F25%23issuecomment-372017942&data=02%7C01%7C%7C86e7b326eb59473c2bcc08d5866d1c38%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C636562725538406478&sdata=V0KmABLKemOmLQlDDpQysW%2BMGi0aXn9H9l5I%2Bf216dA%3D&reserved=0>, or mute the thread<https://eur02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAiFoIb5NLuZBC4v1uQXCJaE4Cz8PpNpCks5tc6MlgaJpZM4SkkVn&data=02%7C01%7C%7C86e7b326eb59473c2bcc08d5866d1c38%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C636562725538406478&sdata=x0jYy8MIeIhjk1Hk0igKEF7yBAK8sswnPtdHK7qNgFU%3D&reserved=0>.

JuanBSLeite · 2018-03-10T17:27:24Z

Hi Augusto,

I did the changes you said and updated my fork. I'll do the new PR now.

henryiii · 2018-03-10T17:33:47Z

The PR gets updated automatically based on your branch (including master); on Monday I can show you how to squash and clean your commit history if you'd like.

AAAlvesJr · 2018-03-10T18:07:09Z

Hi Juan, thanks.
Tell me one thing, how does this implementation compares to the original code you used as inspiration? Did you tested if they return the same values?
Other point: did you compared your implementation to the formulas here described in
CERN-EP-PHYS-76-8 ?

AAAlvesJr · 2018-03-19T06:53:47Z

CMakeLists.txt


 if(Minuit2_FOUND)
-add_subdirectory(examples/phys)


All examples in the directory '''phys''' depends on Minuit2.
You need to put it inside the guards ''' if(Minuit2_FOUND) ... endif(Minuit2_FOUND)'''

AAAlvesJr · 2018-03-19T06:54:52Z

examples/phys/FlatteLineShape.inl

@@ -350,7 +350,7 @@ int main(int argv, char** argc)

    double f0_MASS        = 0.965;
 	double f0_MAG		  = 12.341;
-	double f0_Phase		  = -62.852*(M_PI / 180) ;


Document the numerical values, please.

AAAlvesJr · 2018-03-19T06:57:16Z

examples/phys/FlatteLineShape.inl

@@ -350,7 +350,7 @@ int main(int argv, char** argc)

    double f0_MASS        = 0.965;
 	double f0_MAG		  = 12.341;
-	double f0_Phase		  = -62.852*(M_PI / 180) ;
+	double f0_Phase		  = -62.852*(M_PI / 180) +M_PI;


The primary documentation of code, is the code itself. So, please enforce double precision syntax where you are using it. For example '''180''' -> '''180.0'''

AAAlvesJr · 2018-03-19T06:58:37Z

examples/phys/FlatteLineShape.inl

-
-    auto coef_ref0 = hydra::Parameter::Create().Name("f0_RC").Value(f0_RC).Error(0.0001);
-    auto coef_imf0 = hydra::Parameter::Create().Name("f0_IM").Value(f0_IMC).Error(0.0001);
+    auto coef_ref0 = hydra::Parameter::Create().Name("f0_RC").Value(f0_RC).Error(0.0001).Limits(-100,100);


Define the limits in the parameters in terms of the nominal values in the parameters.

AAAlvesJr · 2018-03-19T07:12:24Z

hydra/functions/FlatteLineShape.h

@@ -218,9 +218,9 @@ namespace hydra {


                if(s >= twopimasssq)
-                    rhopipi_real = (2. / 3) * TMath::Sqrt(1 - twopimasssq / s ); // Above pi+pi- threshold
+                    rhopipi_real = (2. / 3) * ::sqrt(1 - twopimasssq / s ); // Above pi+pi- threshold


This whole piece of code could generate branching and does unnecessary variable redefinition, which makes execution slow down on gpus. I consider a defect of C++ that variables are not default const, like in Rust, but this is another discussion. The code is also confusing. So deploy Boolean arithmetic here. Example:

instead to write

if(condition) x = a+b; else x=a-b;

write (1 line !)

x = a + (condition==true) *(b) - (condition!=true) *(b)

and so on...
Please, reflect about this.

AAAlvesJr · 2018-03-19T07:17:57Z

hydra/functions/FlatteLineShape.h

                else
-                    rhopipi_imag = (2. / 3) * TMath::Sqrt(-1 + twopimasssq / s);
+                    rhopipi_imag = (2. / 3) * ::sqrt(-1 + twopimasssq / s);


AAAlvesJr · 2018-03-19T07:19:32Z

Hi Juan, thanks.
There are still some minor issues. I commented them inline. please address them and let me know. Thanks.

AAAlvesJr · 2018-03-19T09:13:26Z

hydra/functions/FlatteLineShape.h

@@ -223,19 +223,19 @@ namespace hydra {
                    rhopipi_imag = (2. / 3) * ::sqrt(-1 + twopimasssq / s);


Please, try to eliminate the if/else instructions using boolean arithmetic. Avoid value., enforce value.0.

AAAlvesJr · 2018-03-19T09:17:18Z

One more point, I forgot to comment previously. The getter and setters should reflect the names of the members they refer to. For example: double fMass -> void SetMass(double mass){...}

AAAlvesJr · 2018-04-06T08:26:35Z

hydra/functions/FlatteLineShape.h

@@ -78,11 +79,13 @@ namespace hydra {
 * @param daugther3_mass daughter particle 2 mass
 * @param radi decay vertex radio.
 */
-        FlatteLineShape(hydra::Parameter const& mean, hydra::Parameter const& rho1 , hydra::Parameter const& rho2,


What is the length of the vector std::vector<std::vector<double>> params and what does it holds?
I think can be a good idea to document the meaning of this parameter.
Also, are these parameters supposed to be optimized in a fit? Case yes, consider to use the correct interface to store and make them "trackable" by Minuit2 and copy constructors on device and host sides.
If the number of such parameters is not fixed, but still constant at compile time, please, add a template parameter to the class to implement then, and set the default value to the most common case (my guess is most of the time it is 2...) . After set them using this->SetParameter(param_index, param_value) in the body of the constructor.

AAAlvesJr · 2018-04-06T08:30:40Z

hydra/functions/FlatteLineShape.h

                        double mother_mass,
                        double daugther1_mass, double daugther2_mass, double daugther3_mass,
                        double radi) :
-                BaseFunctor<FlatteLineShape<CHANNEL, ArgIndex>, hydra::complex<double>, 3>{mean, rho1, rho2},
+                BaseFunctor<FlatteLineShape<CHANNEL, ArgIndex>, hydra::complex<double>,1>{mean},
+                fParams(params),


Unfortunately, this implementation does not fit the Hydra's requirements, since this class is not copyable on device side for CUDA backends. This happens because you are using a std::vector as a member. This will works on host and device CPU-based backends, but not in CUDA, because std::vector are not implemented there...

AAAlvesJr · 2018-04-06T08:33:22Z

hydra/functions/FlatteLineShape.h

+        }
+
+        __hydra_host__  __hydra_device__ inline
+        void SetDaughter1Mass(std::vector<std::vector<double>> _params) {


it seems to me this member is setting the wrong class member... Should be fDaughterMass1 instead of fParams, right?

AAAlvesJr · 2018-04-06T08:35:07Z

hydra/functions/FlatteLineShape.h

@@ -256,6 +238,7 @@ namespace hydra {
        double fBachelorMass;
        double fMotherMass;
        double fRadi;
+        std::vector<std::vector<double>> fParams;


this member is not copyable on CUDA...

AAAlvesJr · 2018-04-06T08:37:23Z

Hi Juan, thanks for this contribution. It looks much better now! Indeed, the EvtGen's implementation is much more reliable...
Please, find my comments inline.
Cheers
A,A,

AAAlvesJr · 2018-04-16T08:12:24Z

Hi Juan, let me confess I am a bit intrigued... How does this functor can be running in CUDA, if the member std::array<...> fParams is not copyable in that back-end?

As I told you personally a pair of times, if the thresholds are always fixed parameters, it is not necessary to pass them to the hydra::BaseFunctor interface, only the parameters that need be tracked during a fit need be passed...

Look forward for your feedback...
A.A.

JuanBSLeite · 2018-04-17T11:00:38Z

Hi Augusto, std::array is a container to simple C array, it allow use iterators and other things, that's why I think it is running .
params is not inside the base functor, there is only one parameter there, that is the mean.

BaseFunctor<FlatteLineShape<CHANNEL, ArgIndex>, hydra::complex<double>,1>{mean}

AAAlvesJr · 2018-04-17T11:04:57Z

hydra/functions/FlatteLineShape.h

@@ -238,7 +240,7 @@ for(size_t i = 0; i < fParams.size() ; i++) {
        double fBachelorMass;
        double fMotherMass;
        double fRadi;
-        std::vector<std::vector<double>> fParams;
+        std::array<std::array<double,3>,2> fParams;


This member, std::array<std::array<double,3>,2> fParams; is not copyable in CUDA. AFAIK, there is no implementation of std::array in CUDA. Please, use double fParams[6]

AAAlvesJr · 2018-04-17T11:13:55Z

hydra/functions/FlatteLineShape.h

@@ -121,12 +121,12 @@ namespace hydra {
        }


L.110 : BaseFunctor<FlatteLineShape<CHANNEL, ArgIndex>, hydra::complex, 7>::operator=(other);**
This should be 1 -------------------------------------------------------------------------------------------^^^

This assignment operator can't compile either.

AAAlvesJr · 2018-04-17T11:26:00Z

Please, look my last comments.

The CUDA9 Programing Guide, section F.3.8, states:

F.3.8. Standard Library
Standard libraries are only supported in host code, but not in device code, unless specified otherwise.

AFAIK, the only Standard Library class supported is std::initializer_list, which is explained in the section F.3.15.2. std::initializer_list.

I am still wondering how your code can run on CUDA...

JuanBSLeite · 2018-04-17T11:38:01Z

Can you try to run it in your machine?

henryiii · 2018-04-17T11:44:08Z

I think std::array works if you use --expt-relaxed-constexpr (which you do), since the key methods are constexpr. I'm pretty sure I've used it on device before. I could be wrong, though.

Edit: From looking at the descriptions of std::initializer_list and the CUDA source, I don't see why it would be supported, though...

PS: std::move and std::forward are also explicitly supported (though they are not classes).

AAAlvesJr · 2018-04-17T12:07:06Z

The Standard is clear enough about std::array, as so is the CUDA Programing Guide... Lets not drag ahead this discussion.
In summary, the class is not copyable in CUDA. Please, exchange it by a C-style array.

AAAlvesJr · 2018-04-17T12:13:51Z

BTW, about relying on --expt-relaxed-constexpr for certain things

Programming Guide - v9.1.85
F.3.15.4. Constexpr functions and function templates

By default, a constexpr function cannot be called from a function with incompatible execution space 14. The experimental nvcc flag --expt-relaxed-constexpr removes this restriction. When this flag is specified, host code can invoke a __device__ constexpr function and device code can invoke a __host__ constexpr function. nvcc will define the macro __CUDACC_RELAXED_CONSTEXPR__ when --expt-relaxed-constexpr has been specified. Note that a function template instantiation may not be a constexpr function even if the corresponding template is marked with the keyword constexpr (C++11 Standard Section [dcl.constexpr.p6]).

AAAlvesJr · 2018-06-13T10:56:35Z

ping...

JuanBSLeite added 4 commits March 7, 2018 19:04

first commit - Flatte Lineshape

2bcf807

testing flatté

a13c230

flatte lineshape

7eaaf92

clear unused code

301b2c2

cleaning some code

1cffbcd

flatte

6d59a21

AAAlvesJr reviewed Mar 19, 2018

View reviewed changes

JuanBSLeite added 3 commits March 19, 2018 09:37

changes for PR

6413653

changes for PR

311ffa2

last change to PR

771d054

AAAlvesJr reviewed Mar 19, 2018

View reviewed changes

JuanBSLeite and others added 5 commits March 22, 2018 17:58

if/else to boolean operations

1299a65

How to use the Bolean algebra...

d7550a5

boolean operation

d5de8fa

negative sqrt correction/.//

123edd0

FlatteLineshape based in EvtGen

3a2fe66

AAAlvesJr reviewed Apr 6, 2018

View reviewed changes

JuanBSLeite added 2 commits April 12, 2018 21:51

Flatte running in cuda backend

49d88c9

flatte unused paramaters removed

8bba5bd

AAAlvesJr reviewed Apr 17, 2018

View reviewed changes

AAAlvesJr added the wontfix label Jan 23, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Develop #25

Develop #25

JuanBSLeite commented Mar 9, 2018

AAAlvesJr commented Mar 10, 2018

JuanBSLeite commented Mar 10, 2018 via email

JuanBSLeite commented Mar 10, 2018

henryiii commented Mar 10, 2018

AAAlvesJr commented Mar 10, 2018

AAAlvesJr Mar 19, 2018

AAAlvesJr Mar 19, 2018

AAAlvesJr Mar 19, 2018

AAAlvesJr Mar 19, 2018

AAAlvesJr Mar 19, 2018 •

edited

AAAlvesJr Mar 19, 2018

AAAlvesJr commented Mar 19, 2018

AAAlvesJr Mar 19, 2018

AAAlvesJr commented Mar 19, 2018

AAAlvesJr Apr 6, 2018

AAAlvesJr Apr 6, 2018

AAAlvesJr Apr 6, 2018

AAAlvesJr Apr 6, 2018

AAAlvesJr commented Apr 6, 2018

AAAlvesJr commented Apr 16, 2018

JuanBSLeite commented Apr 17, 2018

AAAlvesJr Apr 17, 2018

AAAlvesJr Apr 17, 2018

AAAlvesJr commented Apr 17, 2018

JuanBSLeite commented Apr 17, 2018

henryiii commented Apr 17, 2018 •

edited

AAAlvesJr commented Apr 17, 2018 •

edited

AAAlvesJr commented Apr 17, 2018

AAAlvesJr commented Jun 13, 2018

		@@ -223,19 +223,19 @@ namespace hydra {
		rhopipi_imag = (2. / 3) * ::sqrt(-1 + twopimasssq / s);


		if(Minuit2_FOUND)
		add_subdirectory(examples/phys)

Develop #25

Are you sure you want to change the base?

Develop #25

Conversation

JuanBSLeite commented Mar 9, 2018

AAAlvesJr commented Mar 10, 2018

JuanBSLeite commented Mar 10, 2018 via email

JuanBSLeite commented Mar 10, 2018

henryiii commented Mar 10, 2018

AAAlvesJr commented Mar 10, 2018

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

AAAlvesJr Mar 19, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AAAlvesJr commented Mar 19, 2018

Choose a reason for hiding this comment

AAAlvesJr commented Mar 19, 2018

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

AAAlvesJr commented Apr 6, 2018

AAAlvesJr commented Apr 16, 2018

JuanBSLeite commented Apr 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AAAlvesJr commented Apr 17, 2018

JuanBSLeite commented Apr 17, 2018

henryiii commented Apr 17, 2018 • edited

AAAlvesJr commented Apr 17, 2018 • edited

AAAlvesJr commented Apr 17, 2018

AAAlvesJr commented Jun 13, 2018

AAAlvesJr Mar 19, 2018 •

edited

henryiii commented Apr 17, 2018 •

edited

AAAlvesJr commented Apr 17, 2018 •

edited